> 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. > > Nikita Skovoroda wrote: > 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).
I added a check to make sure the video id matches a regex for a valid video id. That way nothing can creep in masquerading as a video id. > 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/)([^&]+)" > > Nikita Skovoroda wrote: > 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). At this stage I don't want to support youtube.be, although I did add a check for youtube.com (without the www) URLs. The usecase for this plugin is when you copy-paste the video of a URL that you are watching to a friend. Since youtube _allways_ expands to http://www.youtube.com/, there's no point in supporting the former two. > On Sept. 2, 2012, 12:04 a.m., Nikita Skovoroda wrote: > > filters/youtube/youtube-filter.cpp, line 58 > > <http://git.reviewboard.kde.org/r/106083/diff/1/?file=78685#file78685line58> > > > > What happens if «v» parameter is empty? > > Nikita Skovoroda wrote: > Parsing the url with regular expressions (see above) would solve this > with no extra code involved. it won't match the regex. - Lasath ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106083/#review18419 ----------------------------------------------------------- On Sept. 11, 2012, 10:19 p.m., Lasath Fernando wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106083/ > ----------------------------------------------------------- > > (Updated Sept. 11, 2012, 10:19 p.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 > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
