Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1015463004:
Consistent content placement method for dom-distiller (Closed)
DescriptionConsistent content placement method for dom-distiller
This change introduces a consistent method of placing content on
the page (via JavaScript and the function "addToPage()"). The
content is no longer in the HTML template by default. This fixes
a bug with the loading icon not appearing until after the second
page of a multi-page article. Error pages have content placed by a
web new observer.
BUG=
Committed: https://crrev.com/83653dd1da59dfa7ddd9e48d4cd507a11cefd968
Cr-Commit-Position: refs/heads/master@{#323527}
Patch Set 1 #Patch Set 2 : Nits and cl format #Patch Set 3 : Add test #Patch Set 4 : Bug fix & related cleanup #
Total comments: 2
Patch Set 5 : Template observer #
Total comments: 2
Patch Set 6 : Use single observer #
Total comments: 2
Patch Set 7 : Clean up pointers #Patch Set 8 : Comments #Patch Set 9 : More concise fix #Patch Set 10 : Fix IOS version of code #
Total comments: 4
Patch Set 11 : IOS content fix #
Total comments: 4
Patch Set 12 : Todos #Patch Set 13 : Handle IOS string correctly #Patch Set 14 : Revert ios code #
Messages
Total messages: 45 (15 generated)
mdjones@chromium.org changed reviewers: + cjhopman@chromium.org
https://codereview.chromium.org/1015463004/diff/60001/components/dom_distille... File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/1015463004/diff/60001/components/dom_distille... components/dom_distiller/content/dom_distiller_viewer_source.cc:36: class WebContentsErrorObserver If distillation returned really really fast, could a successful distillation run into the same case of calling javascript too early?
https://codereview.chromium.org/1015463004/diff/60001/components/dom_distille... File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/1015463004/diff/60001/components/dom_distille... components/dom_distiller/content/dom_distiller_viewer_source.cc:36: class WebContentsErrorObserver On 2015/03/21 02:33:22, cjhopman wrote: > If distillation returned really really fast, could a successful distillation run > into the same case of calling javascript too early? Yes, I have added a second observer to watch for the template to be loaded on the page. This places the first page of content then stops observing.
https://codereview.chromium.org/1015463004/diff/80001/components/dom_distille... File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/1015463004/diff/80001/components/dom_distille... components/dom_distiller/content/dom_distiller_viewer_source.cc:309: SendJavaScript(viewer::GetUnsafeIncrementalDistilledPageJs(&page, false)); Isn't it possible now that the second page has it's javascript called before the first page? I feel like we should be able to do this with just one observer and delaying all SendJavaScript() calls (and then just using SendJavaScript as normal for both the error case and the first page).
I have consolidated the two observers but think the potential script execution order problem should be handled in a separate CL since it is a problem that existed prior to this one.
https://codereview.chromium.org/1015463004/diff/80001/components/dom_distille... File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/1015463004/diff/80001/components/dom_distille... components/dom_distiller/content/dom_distiller_viewer_source.cc:309: SendJavaScript(viewer::GetUnsafeIncrementalDistilledPageJs(&page, false)); On 2015/03/24 01:45:36, cjhopman wrote: > Isn't it possible now that the second page has it's javascript called before the > first page? > > I feel like we should be able to do this with just one observer and delaying all > SendJavaScript() calls (and then just using SendJavaScript as normal for both > the error case and the first page). Done/See general comment.
https://codereview.chromium.org/1015463004/diff/100001/components/dom_distill... File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/1015463004/diff/100001/components/dom_distill... components/dom_distiller/content/dom_distiller_viewer_source.cc:368: WebContentsTemplateObserver* template_observer = Who owns this? When is it deleted?
https://codereview.chromium.org/1015463004/diff/100001/components/dom_distill... File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/1015463004/diff/100001/components/dom_distill... components/dom_distiller/content/dom_distiller_viewer_source.cc:368: WebContentsTemplateObserver* template_observer = On 2015/03/25 02:30:50, cjhopman wrote: > Who owns this? When is it deleted? The observer now deletes itself once the first page is sent, the web contents are destroyed or the renderer is gone.
This is a more concise fix that uses the existing observer rather than a new one.
lgtm
The CQ bit was checked by mdjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015463004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cjhopman@chromium.org Link to the patchset: https://codereview.chromium.org/1015463004/#ps170001 (title: "Fix IOS version of code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015463004/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mdjones@chromium.org changed reviewers: + noyau@chromium.org
+noyau I have added you to the list of reviewers because my changes require affect some of the IOS specific code. Please take a look at distiller_viewer.cc.
https://codereview.chromium.org/1015463004/diff/170001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right): https://codereview.chromium.org/1015463004/diff/170001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/distiller_viewer.cc:42: &article_proto->pages(0), distilled_page_prefs_->GetTheme(), It looks like your change will make it so iOS doesn't get any of the data (as they were relying on it all being in the initial html). One approach that might give them the content is to append a <script> tag that contains the javascript to call for all of the pages.
https://codereview.chromium.org/1015463004/diff/170001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right): https://codereview.chromium.org/1015463004/diff/170001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/distiller_viewer.cc:42: &article_proto->pages(0), distilled_page_prefs_->GetTheme(), On 2015/03/27 00:45:15, cjhopman wrote: > It looks like your change will make it so iOS doesn't get any of the data (as > they were relying on it all being in the initial html). > > One approach that might give them the content is to append a <script> tag that > contains the javascript to call for all of the pages. I don't see the code that uses this for iOS (maybe it's internal) so I don't know what else might work. noyau@ probably has an idea. If they can call javascript in the page when the callback is run, maybe the callback could take both the html and a bit of javascript to call to insert the content.
https://codereview.chromium.org/1015463004/diff/170001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right): https://codereview.chromium.org/1015463004/diff/170001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/distiller_viewer.cc:42: &article_proto->pages(0), distilled_page_prefs_->GetTheme(), On 2015/03/27 00:45:15, cjhopman wrote: > It looks like your change will make it so iOS doesn't get any of the data (as > they were relying on it all being in the initial html). > > One approach that might give them the content is to append a <script> tag that > contains the javascript to call for all of the pages. Done, but this should be considered a temporary fix. https://codereview.chromium.org/1015463004/diff/170001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/distiller_viewer.cc:42: &article_proto->pages(0), distilled_page_prefs_->GetTheme(), On 2015/03/27 00:48:33, cjhopman wrote: > On 2015/03/27 00:45:15, cjhopman wrote: > > It looks like your change will make it so iOS doesn't get any of the data (as > > they were relying on it all being in the initial html). > > > > One approach that might give them the content is to append a <script> tag that > > contains the javascript to call for all of the pages. > > I don't see the code that uses this for iOS (maybe it's internal) so I don't > know what else might work. noyau@ probably has an idea. If they can call > javascript in the page when the callback is run, maybe the callback could take > both the html and a bit of javascript to call to insert the content. Acknowledged.
https://codereview.chromium.org/1015463004/diff/190001/components/dom_distill... File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/1015463004/diff/190001/components/dom_distill... components/dom_distiller/content/dom_distiller_viewer_source.cc:232: } This logic needs to be moved into a common superclass. Building the html and the javascript should be completely agnostic of the platform and the call to the underline content could be abstracted (They already mostly are). All the javascript buffering should also be moved up. The error page also can be moved up. https://codereview.chromium.org/1015463004/diff/190001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right): https://codereview.chromium.org/1015463004/diff/190001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/distiller_viewer.cc:48: html += "<script>" + content_js + "</script>"; This is definitely not the right way to do this. The proper approach would be to change the DistillationFinishedCallback so it takes both the HTML and the javascript to inject. This will require changes in the client code that is not in chromium, so leave it there but with a TODO(noyau) and an associated bug. Note that on iOS the equivalent of content that is used to render the UI is not created right away (the distillation may be happening in the background and only if the user tap on a confirmation dialog will the webview be created). This helps a lot with memory management (webviews are expensive).
https://codereview.chromium.org/1015463004/diff/190001/components/dom_distill... File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): https://codereview.chromium.org/1015463004/diff/190001/components/dom_distill... components/dom_distiller/content/dom_distiller_viewer_source.cc:232: } On 2015/04/01 11:42:08, noyau wrote: > This logic needs to be moved into a common superclass. Building the html and the > javascript should be completely agnostic of the platform and the call to the > underline content could be abstracted (They already mostly are). All the > javascript buffering should also be moved up. > > The error page also can be moved up. I have marked this as a TODO for myself. https://codereview.chromium.org/1015463004/diff/190001/ios/chrome/browser/dom... File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right): https://codereview.chromium.org/1015463004/diff/190001/ios/chrome/browser/dom... ios/chrome/browser/dom_distiller/distiller_viewer.cc:48: html += "<script>" + content_js + "</script>"; On 2015/04/01 11:42:08, noyau wrote: > This is definitely not the right way to do this. > > The proper approach would be to change the DistillationFinishedCallback so it > takes both the HTML and the javascript to inject. This will require changes in > the client code that is not in chromium, so leave it there but with a > TODO(noyau) and an associated bug. > > Note that on iOS the equivalent of content that is used to render the UI is not > created right away (the distillation may be happening in the background and only > if the user tap on a confirmation dialog will the webview be created). This > helps a lot with memory management (webviews are expensive). Done.
The CQ bit was checked by cjhopman@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from cjhopman@chromium.org Link to the patchset: https://codereview.chromium.org/1015463004/#ps210001 (title: "Todos")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015463004/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by mdjones@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from cjhopman@chromium.org Link to the patchset: https://codereview.chromium.org/1015463004/#ps230001 (title: "Handle IOS string correctly")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015463004/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cjhopman@chromium.org Link to the patchset: https://codereview.chromium.org/1015463004/#ps250001 (title: "Revert ios code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015463004/250001
Message was sent while issue was closed.
Committed patchset #14 (id:250001)
Message was sent while issue was closed.
On 2015/04/01 22:25:34, mdjones wrote: > https://codereview.chromium.org/1015463004/diff/190001/components/dom_distill... > File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): > > https://codereview.chromium.org/1015463004/diff/190001/components/dom_distill... > components/dom_distiller/content/dom_distiller_viewer_source.cc:232: } > On 2015/04/01 11:42:08, noyau wrote: > > This logic needs to be moved into a common superclass. Building the html and > the > > javascript should be completely agnostic of the platform and the call to the > > underline content could be abstracted (They already mostly are). All the > > javascript buffering should also be moved up. > > > > The error page also can be moved up. > > I have marked this as a TODO for myself. > > https://codereview.chromium.org/1015463004/diff/190001/ios/chrome/browser/dom... > File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right): > > https://codereview.chromium.org/1015463004/diff/190001/ios/chrome/browser/dom... > ios/chrome/browser/dom_distiller/distiller_viewer.cc:48: html += "<script>" + > content_js + "</script>"; > On 2015/04/01 11:42:08, noyau wrote: > > This is definitely not the right way to do this. > > > > The proper approach would be to change the DistillationFinishedCallback so it > > takes both the HTML and the javascript to inject. This will require changes in > > the client code that is not in chromium, so leave it there but with a > > TODO(noyau) and an associated bug. > > > > Note that on iOS the equivalent of content that is used to render the UI is > not > > created right away (the distillation may be happening in the background and > only > > if the user tap on a confirmation dialog will the webview be created). This > > helps a lot with memory management (webviews are expensive). > > Done. I'm confused why you reverted all the iOS code?
Message was sent while issue was closed.
I will resubmit the IOS stuff once all the feedback changes that depended on this land; I was blocked on no lgtm. On Thu, Apr 2, 2015 at 10:53 AM, <noyau@chromium.org> wrote: > On 2015/04/01 22:25:34, mdjones wrote: > > https://codereview.chromium.org/1015463004/diff/190001/ > components/dom_distiller/content/dom_distiller_viewer_source.cc > >> File components/dom_distiller/content/dom_distiller_viewer_source.cc >> (right): >> > > > https://codereview.chromium.org/1015463004/diff/190001/ > components/dom_distiller/content/dom_distiller_viewer_source.cc#newcode232 > >> components/dom_distiller/content/dom_distiller_viewer_source.cc:232: } >> On 2015/04/01 11:42:08, noyau wrote: >> > This logic needs to be moved into a common superclass. Building the >> html and >> the >> > javascript should be completely agnostic of the platform and the call >> to the >> > underline content could be abstracted (They already mostly are). All the >> > javascript buffering should also be moved up. >> > >> > The error page also can be moved up. >> > > I have marked this as a TODO for myself. >> > > > https://codereview.chromium.org/1015463004/diff/190001/ > ios/chrome/browser/dom_distiller/distiller_viewer.cc > >> File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right): >> > > > https://codereview.chromium.org/1015463004/diff/190001/ > ios/chrome/browser/dom_distiller/distiller_viewer.cc#newcode48 > >> ios/chrome/browser/dom_distiller/distiller_viewer.cc:48: html += >> "<script>" + >> content_js + "</script>"; >> On 2015/04/01 11:42:08, noyau wrote: >> > This is definitely not the right way to do this. >> > >> > The proper approach would be to change the DistillationFinishedCallback >> so >> > it > >> > takes both the HTML and the javascript to inject. This will require >> changes >> > in > >> > the client code that is not in chromium, so leave it there but with a >> > TODO(noyau) and an associated bug. >> > >> > Note that on iOS the equivalent of content that is used to render the >> UI is >> not >> > created right away (the distillation may be happening in the background >> and >> only >> > if the user tap on a confirmation dialog will the webview be created). >> This >> > helps a lot with memory management (webviews are expensive). >> > > Done. >> > > I'm confused why you reverted all the iOS code? > > > https://codereview.chromium.org/1015463004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/04/02 17:53:21, noyau wrote: > On 2015/04/01 22:25:34, mdjones wrote: > > > https://codereview.chromium.org/1015463004/diff/190001/components/dom_distill... > > File components/dom_distiller/content/dom_distiller_viewer_source.cc (right): > > > > > https://codereview.chromium.org/1015463004/diff/190001/components/dom_distill... > > components/dom_distiller/content/dom_distiller_viewer_source.cc:232: } > > On 2015/04/01 11:42:08, noyau wrote: > > > This logic needs to be moved into a common superclass. Building the html and > > the > > > javascript should be completely agnostic of the platform and the call to the > > > underline content could be abstracted (They already mostly are). All the > > > javascript buffering should also be moved up. > > > > > > The error page also can be moved up. > > > > I have marked this as a TODO for myself. > > > > > https://codereview.chromium.org/1015463004/diff/190001/ios/chrome/browser/dom... > > File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right): > > > > > https://codereview.chromium.org/1015463004/diff/190001/ios/chrome/browser/dom... > > ios/chrome/browser/dom_distiller/distiller_viewer.cc:48: html += "<script>" + > > content_js + "</script>"; > > On 2015/04/01 11:42:08, noyau wrote: > > > This is definitely not the right way to do this. > > > > > > The proper approach would be to change the DistillationFinishedCallback so > it > > > takes both the HTML and the javascript to inject. This will require changes > in > > > the client code that is not in chromium, so leave it there but with a > > > TODO(noyau) and an associated bug. > > > > > > Note that on iOS the equivalent of content that is used to render the UI is > > not > > > created right away (the distillation may be happening in the background and > > only > > > if the user tap on a confirmation dialog will the webview be created). This > > > helps a lot with memory management (webviews are expensive). > > > > Done. > > I'm confused why you reverted all the iOS code? I didn't want this blocked on making those iOS changes as this is blocking several other things. I believe that matt will be uploading a CL that contains the iOS changes that were in PS#13.
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/83653dd1da59dfa7ddd9e48d4cd507a11cefd968 Cr-Commit-Position: refs/heads/master@{#323527} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
