Review Request: So master isn't frozen anymore...

Nikita Skovoroda chalkerx at gmail.com
Mon Sep 3 07:49:59 UTC 2012 


> On Sept. 2, 2012, 12:04 a.m., Nikita Skovoroda wrote:
> > filters/youtube/youtube-filter.cpp, line 47
> > <http://git.reviewboard.kde.org/r/106083/diff/1/?file=78685#file78685line47>
> >
> >     The %1 = url.queryItemValue(QLatin1String("v")) is not escaped here, and can contain ",<,> symbols (and any other complex html).
> >     
> >     Don't forget that queryItemValue decodes percent-encoded strings.
> 
> Nikita Skovoroda wrote:
>     I'd make a separate variable for the link and escape it using KUrl.
>     Crude example follows:
>     
>     html template:
>         …iframe src=\"%1\"…
>     url:
>         KUrl frameUrl(QLatin1String("http://www.youtube.com/embed/%1").arg(id)); 
>     inserting the url into the html template:
>         .arg(QString::fromAscii(frameUrl.toEncoded()))
>     
>     Or something like that. That should fix things.

I missed one thing: ID can contain something like percent-encoded "../html5" string, and http://www.youtube.com/html5 would open inside the iframe in this case. That's not good, either.

Maybe it would be better to just pass the id through QUrl::toPercentEncoding and use the original template, but you need to specify what symbols to keep depending on how you extracted the id.
If the id was extracted using queryItemValue, then you want just to encode everything (because it decodes everything that was previosly encoded).
If the id was extracted using a regexp, then you probably wish to keep the % symbols (because you don't want to do double-encoding in case if it's already done).


> On Sept. 2, 2012, 12:04 a.m., Nikita Skovoroda wrote:
> > filters/youtube/youtube-filter.cpp, line 54
> > <http://git.reviewboard.kde.org/r/106083/diff/1/?file=78685#file78685line54>
> >
> >     What happens to http://youtube.com/watch?v=someid ?
> 
> Nikita Skovoroda wrote:
>     Another possible format: http://youtu.be/someid , this is equivalent to http://www.youtube.com/watch?v=someid
> 
> Nikita Skovoroda wrote:
>     You could add an array/list of structs, for example:
>     
>     {
>      QString urlRegexp; // Url for matching and extracting the video id
>      QString urlSubstitute; // Url for embedding, for example, iframe url
>      QString html; // The html template that contains the code that needs to be embeded.
>     }
>     
>     This way, it would be easy to support different video hosting services and several possible prefixes for each one.
>     
>     For example, youtube would have:
>     
>     urlRegexp = "^(?:https?://)?(?:www\.)(?:youtube\.com/watch.*[&\?]v=|youtu\.be/)([^&]+)" // Or something like that, i haven't checked this one, written down without testing.
>     urlSubstitute = "http://www.youtube.com/embed/%1"
>     html = "…iframe src=\"%1\"…" // Your html, it's too big to include it here.
> 
> Nikita Skovoroda wrote:
>     Forgot an «?» after «(?:www\.)»:
>     urlRegexp = "^(?:https?://)?(?:www\.)?(?:youtube\.com/watch.*[&\?]v=|youtu\.be/)([^&]+)"

My wrong, separation of urlSubstitute and html parameters is not needed, because the video id can be encoded separately as described above.
So, the whole thing could just be a QMap(fromRegex => toHtmlTemplate).


- Nikita


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106083/#review18419
-----------------------------------------------------------


On Aug. 19, 2012, 10:29 a.m., Lasath Fernando wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106083/
> -----------------------------------------------------------
> 
> (Updated Aug. 19, 2012, 10:29 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Right, so here's a big fat review containing many many things, including all the remaining plugins and a few tweaks to Message.
> 
> As per usual, all the code is in my scratch repo: http://quickgit.kde.org/index.php?p=clones%2Fktp-text-ui%2Ffernando%2Fgsoc.git&a=summary
> 
> Now, I'm going to take a well earned nap.
> 
> 
> Diffs
> -----
> 
>   filters/CMakeLists.txt ee7c23d 
>   filters/bugzilla/CMakeLists.txt PRE-CREATION 
>   filters/bugzilla/bugzilla-filter.h PRE-CREATION 
>   filters/bugzilla/bugzilla-filter.cpp PRE-CREATION 
>   filters/bugzilla/ktptextui_message_filter_bugzilla.desktop PRE-CREATION 
>   filters/highlight/CMakeLists.txt PRE-CREATION 
>   filters/highlight/highlight-filter.h PRE-CREATION 
>   filters/highlight/highlight-filter.cpp PRE-CREATION 
>   filters/highlight/ktptextui_message_filter_highlight.desktop PRE-CREATION 
>   filters/latex/CMakeLists.txt PRE-CREATION 
>   filters/latex/ktp_message_filter_latex_converter.sh PRE-CREATION 
>   filters/latex/ktptextui_message_filter_latex.desktop PRE-CREATION 
>   filters/latex/latex-filter.h PRE-CREATION 
>   filters/latex/latex-filter.cpp PRE-CREATION 
>   filters/pipes/CMakeLists.txt PRE-CREATION 
>   filters/pipes/kcm_ktp_filter_config_pipes.desktop PRE-CREATION 
>   filters/pipes/ktptextui_message_filter_pipes.desktop PRE-CREATION 
>   filters/pipes/pipes-config.h PRE-CREATION 
>   filters/pipes/pipes-config.cpp PRE-CREATION 
>   filters/pipes/pipes-config.ui PRE-CREATION 
>   filters/pipes/pipes-delegate.h PRE-CREATION 
>   filters/pipes/pipes-delegate.cpp PRE-CREATION 
>   filters/pipes/pipes-filter.h PRE-CREATION 
>   filters/pipes/pipes-filter.cpp PRE-CREATION 
>   filters/pipes/pipes-model.h PRE-CREATION 
>   filters/pipes/pipes-model.cpp PRE-CREATION 
>   filters/pipes/pipes-prefs.h PRE-CREATION 
>   filters/pipes/pipes-prefs.cpp PRE-CREATION 
>   filters/pipes/pipes-prefstest.h PRE-CREATION 
>   filters/pipes/pipes-prefstest.cpp PRE-CREATION 
>   filters/substitution/CMakeLists.txt PRE-CREATION 
>   filters/substitution/kcm_ktp_filter_config_substitution.desktop PRE-CREATION 
>   filters/substitution/ktptextui_message_filter_substitution.desktop PRE-CREATION 
>   filters/substitution/substitution-config.h PRE-CREATION 
>   filters/substitution/substitution-config.cpp PRE-CREATION 
>   filters/substitution/substitution-config.ui PRE-CREATION 
>   filters/substitution/substitution-filter.h PRE-CREATION 
>   filters/substitution/substitution-filter.cpp PRE-CREATION 
>   filters/substitution/substitution-prefs.h PRE-CREATION 
>   filters/substitution/substitution-prefs.cpp PRE-CREATION 
>   filters/youtube/CMakeLists.txt PRE-CREATION 
>   filters/youtube/ktptextui_message_filter_youtube.desktop PRE-CREATION 
>   filters/youtube/youtube-filter.h PRE-CREATION 
>   filters/youtube/youtube-filter.cpp PRE-CREATION 
>   lib/abstract-message-filter.h 7b60d48 
>   lib/abstract-message-filter.cpp 2a3a897 
>   lib/message.h ef9530b 
>   lib/message.cpp 6db648e 
>   tests/message-processor-basic-tests.h 7dc99e4 
>   tests/message-processor-basic-tests.cpp 8546605 
> 
> Diff: http://git.reviewboard.kde.org/r/106083/diff/
> 
> 
> Testing
> -------
> 
> Wrote/passed/failed unit tests; talked to myself so much to the point I swear I have mild schizophrenia now. 
> 
> 
> Thanks,
> 
> Lasath Fernando
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120903/816483ae/attachment.html>

More information about the KDE-Telepathy mailing list