Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2730863002:
Expand all Wikipedia sections on distillation. (Closed)
Description[Reading List] Expand all Wikipedia sections on distillation.
Most part of Wikipedia articles is hidden when the page loads.
DOM distiller will eventually handle this issue, but for the time being, this is a workaround.
The workaround set the style to block before distilling the page.
BUG=647667, 699215
Review-Url: https://codereview.chromium.org/2730863002
Cr-Commit-Position: refs/heads/master@{#455186}
Committed: https://chromium.googlesource.com/chromium/src/+/f2b4f5683185e5bfdc90c42f97d49ed3e06ca1f2
Patch Set 1 #Patch Set 2 : update comments #Patch Set 3 : add todo #
Total comments: 8
Patch Set 4 : comments #Patch Set 5 : feedback #
Messages
Total messages: 21 (11 generated)
Description was changed from ========== Expand all Wikipedia sections on distillation BUG= ========== to ========== Expand all Wikipedia sections on distillation BUG= ==========
Description was changed from ========== Expand all Wikipedia sections on distillation BUG= ========== to ========== [Reading List] Expand all Wikipedia sections on distillation. Most part of Wikipedia articles is hidden when the page loads. DOM distiller will eventually handle this issue, but for the time being, this is a workaround. The workaround set the stile to block before distilling the page. BUG= ==========
Description was changed from ========== [Reading List] Expand all Wikipedia sections on distillation. Most part of Wikipedia articles is hidden when the page loads. DOM distiller will eventually handle this issue, but for the time being, this is a workaround. The workaround set the stile to block before distilling the page. BUG= ========== to ========== [Reading List] Expand all Wikipedia sections on distillation. Most part of Wikipedia articles is hidden when the page loads. DOM distiller will eventually handle this issue, but for the time being, this is a workaround. The workaround set the stile to block before distilling the page. BUG=647667 ==========
olivierrobin@chromium.org changed reviewers: + jif@chromium.org, wychen@chromium.org
Thanks for the quick fix. This would certainly get more contents, but I'm not entirely sure whether this is better for user experience without further quality enhancements, since images would still be missing, headings not always there, edit links, missing sections if link density is high, etc. In description: stile -> style
Description was changed from ========== [Reading List] Expand all Wikipedia sections on distillation. Most part of Wikipedia articles is hidden when the page loads. DOM distiller will eventually handle this issue, but for the time being, this is a workaround. The workaround set the stile to block before distilling the page. BUG=647667 ========== to ========== [Reading List] Expand all Wikipedia sections on distillation. Most part of Wikipedia articles is hidden when the page loads. DOM distiller will eventually handle this issue, but for the time being, this is a workaround. The workaround set the style to block before distilling the page. BUG=647667 ==========
For now this is fine, but I feel like we are going to need an infrastructure to
be able to more easily handle special cases. Something like:
specialCases = {
{ isGoogleAmpPage, GoogleAmpPageHandler},
{ isWikipediaPage, wikipediaPageHandler}
};
https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right):
https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:85: //
IsWikipediaPage will determine if the current page is a wikipedia article.
IsWikipediaPage determines if
https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_distiller_page.h:89: void
HandleWikipediaPage();
HandleWikipediaPage sets
https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_distiller_page.h (right): https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_distiller_page.h:85: // IsWikipediaPage will determine if the current page is a wikipedia article. On 2017/03/03 16:46:17, jif wrote: > IsWikipediaPage determines if Done. https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_distiller_page.h:89: void HandleWikipediaPage(); On 2017/03/03 16:46:17, jif wrote: > HandleWikipediaPage sets Done.
noyau@chromium.org changed reviewers: + noyau@chromium.org
https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right): https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_distiller_page.mm:211: }]; This is leftover from the initial hack you forgot to remove right? https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_distiller_page.mm:281: // All wikipedia pages are in the form "https://xxx.wikipedia.org/..." In the cases that interest us [countrycode].m.wikipedia.org is probably more accurate. No need to trigger on desktop pages. Note that there is no issue on iPad, despite going to the same URL.
This CL in DOM distiller could also process folded sections. https://codereview.chromium.org/2726123004/ Sorry, I didn't send this out earlier and it feels a bit duplicated. Do we still want to cherry-pick this to M57?
Patchset #5 (id:80001) has been deleted
No issue. If your fix goes in, I have no problem with not submitting this CL. It is just in case we want a temporary workaround before the real DOM distiller fix. https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read... File ios/chrome/browser/reading_list/reading_list_distiller_page.mm (right): https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_distiller_page.mm:211: }]; On 2017/03/03 21:52:52, noyau wrote: > This is leftover from the initial hack you forgot to remove right? Done. https://codereview.chromium.org/2730863002/diff/40001/ios/chrome/browser/read... ios/chrome/browser/reading_list/reading_list_distiller_page.mm:281: // All wikipedia pages are in the form "https://xxx.wikipedia.org/..." On 2017/03/03 21:52:52, noyau wrote: > In the cases that interest us [countrycode].m.wikipedia.org is probably more > accurate. No need to trigger on desktop pages. Note that there is no issue on > iPad, despite going to the same URL. Done.
lgtm
Description was changed from ========== [Reading List] Expand all Wikipedia sections on distillation. Most part of Wikipedia articles is hidden when the page loads. DOM distiller will eventually handle this issue, but for the time being, this is a workaround. The workaround set the style to block before distilling the page. BUG=647667 ========== to ========== [Reading List] Expand all Wikipedia sections on distillation. Most part of Wikipedia articles is hidden when the page loads. DOM distiller will eventually handle this issue, but for the time being, this is a workaround. The workaround set the style to block before distilling the page. BUG=647667, 699215 ==========
The CQ bit was checked by olivierrobin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488915198385660,
"parent_rev": "a60dc6370942b54087fafd03ef204d2b8e1949db", "commit_rev":
"f2b4f5683185e5bfdc90c42f97d49ed3e06ca1f2"}
Message was sent while issue was closed.
Description was changed from ========== [Reading List] Expand all Wikipedia sections on distillation. Most part of Wikipedia articles is hidden when the page loads. DOM distiller will eventually handle this issue, but for the time being, this is a workaround. The workaround set the style to block before distilling the page. BUG=647667, 699215 ========== to ========== [Reading List] Expand all Wikipedia sections on distillation. Most part of Wikipedia articles is hidden when the page loads. DOM distiller will eventually handle this issue, but for the time being, this is a workaround. The workaround set the style to block before distilling the page. BUG=647667, 699215 Review-Url: https://codereview.chromium.org/2730863002 Cr-Commit-Position: refs/heads/master@{#455186} Committed: https://chromium.googlesource.com/chromium/src/+/f2b4f5683185e5bfdc90c42f97d4... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/f2b4f5683185e5bfdc90c42f97d4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
