|
|
Created:
3 years, 9 months ago by George Joseph Modified:
3 years, 6 months ago Reviewers:
Bernhard Bauer, waffles, jochen (gone - plz use gerrit), laforge, sorin, tommycli, George Joseph CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, kinuko+watch Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEmit error events if the loading of an object element failed
Dispatch onerror/error events in the case of a placeholder being
instantiated instead of an actual plugin.
BUG=445557
Review-Url: https://codereview.chromium.org/2733083004
Cr-Commit-Position: refs/heads/master@{#469081}
Committed: https://chromium.googlesource.com/chromium/src/+/f4335e87b019d74e3d803318101922bb40cec682
Patch Set 1 #
Total comments: 4
Patch Set 2 : Emit error events if the loading of an object element failed #Patch Set 3 : Emit error events if the loading of an object element failed #
Total comments: 2
Patch Set 4 : Emit error events if the loading of an object element failed #
Total comments: 6
Patch Set 5 : Emit error events if the loading of an object element failed #
Total comments: 10
Patch Set 6 : Emit error events if the loading of an object element failed #
Total comments: 7
Patch Set 7 : Emit error events if the loading of an object element failed #
Total comments: 11
Patch Set 8 : Emit error events if the loading of an object element failed #
Total comments: 14
Patch Set 9 : Emit error events if the loading of an object element failed #
Total comments: 4
Patch Set 10 : Emit error events if the loading of an object element failed #
Total comments: 8
Patch Set 11 : Emit error events if the loading of an object element failed #
Total comments: 6
Patch Set 12 : Emit error events if the loading of an object element failed #
Total comments: 8
Patch Set 13 : Emit error events if the loading of an object element failed #
Total comments: 4
Patch Set 14 : Emit error events if the loading of an object element failed #
Total comments: 2
Patch Set 15 : Emit error events if the loading of an object element failed #Patch Set 16 : Emit error events if the loading of an object element failed #Patch Set 17 : Emit error events if the loading of an object element failed #Messages
Total messages: 103 (36 generated)
Description was changed from ========== Emit error events if the loading of an object element failed Dispatch onerror/error events in the case of a placeholder being instantiated instead of an actual plugin. BUG=445557 ========== to ========== Emit error events if the loading of an object element failed Dispatch onerror/error events in the case of a placeholder being instantiated instead of an actual plugin. BUG=445557 ==========
kottackal.george@gmail.com changed reviewers: + bauerb@chromium.org, tommycli@chromium.org
The CQ bit was checked by kottackal.george@gmail.com to run a CQ dry run
The CQ bit was unchecked by kottackal.george@gmail.com
The CQ bit was checked by kottackal.george@gmail.com
The CQ bit was unchecked by kottackal.george@gmail.com
The CQ bit was unchecked by kottackal.george@gmail.com
The CQ bit was checked by kottackal.george@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
kottackal.george@gmail.com changed reviewers: - Kottackal.George@gmail.com
https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... File components/plugins/renderer/loadable_plugin_placeholder.cc (right): https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... components/plugins/renderer/loadable_plugin_placeholder.cc:75: plugin()->markAsPlaceholder(); If there is a way that the plugin might run eventually (e.g. click-to-play), we should _not_ emit an error event. It is by design that the page can't distinguish a blocked plugin from one that just takes a reeeallly long time to load. OTOH, if we know that the plugin will never load (e.g. enterprise policy), then yeah, we can notify the page. Tommy, WDYT?
Hello Bernhard, Thanks for reviewing the change. Please do review my responses. Thanks, George. https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... File components/plugins/renderer/loadable_plugin_placeholder.cc (right): https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... components/plugins/renderer/loadable_plugin_placeholder.cc:75: plugin()->markAsPlaceholder(); On 2017/03/08 14:03:14, Bernhard Bauer wrote: > If there is a way that the plugin might run eventually (e.g. click-to-play), we > should _not_ emit an error event. It is by design that the page can't > distinguish a blocked plugin from one that just takes a reeeallly long time to > load. > > OTOH, if we know that the plugin will never load (e.g. enterprise policy), then > yeah, we can notify the page. Tommy, WDYT? Hello Bernhard, Thanks for reviewing the change.The original bug was raised because no events were being fired when a plugin could not be loaded.In the added test case , that is the scenario I have replicated - a mimetype for which a Plugin cannot be loaded has been added. Currently, Firefox does fire an error event, if a plugin for a mimetype cannot be loaded. Also , it does fire an error event if a plugin is blocked (even if a plugin can be created for the mimetype). On unblocking the plugin , firefox does not fire a load event on the object. My debugging so far has revealed that the ChromePluginPlaceholder is instantiated only when a plugin cannot be created from the existing mimetype registry, this is so that a plugin object will always be created irrespective of whether the plugin is supported or not so that a Widget always gets instantiated (even if the mimetype is not supported) - I am happy to be corrected on this. Please note that I could not add code to send load events, because ideally, once a plugin is loaded I would think that it is the plugins responsibility to fire events if event handlers are registered.It can be done in the HTMLObjectElement , but then if a plugin fires its load handler, after a load it would result in mutilple events for one load.For the purpose of the bug, I think the onus of dispatching load events should be on the plugin. Do let me know your thoughts, Thanks, George.
Hello Bernhard, Thanks for reviewing the change. Please do review my responses. Thanks, George.
bauerb@chromium.org changed reviewers: + groby@chromium.org
https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... File components/plugins/renderer/loadable_plugin_placeholder.cc (right): https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... components/plugins/renderer/loadable_plugin_placeholder.cc:75: plugin()->markAsPlaceholder(); On 2017/03/08 14:45:57, George Joseph wrote: > On 2017/03/08 14:03:14, Bernhard Bauer wrote: > > If there is a way that the plugin might run eventually (e.g. click-to-play), > we > > should _not_ emit an error event. It is by design that the page can't > > distinguish a blocked plugin from one that just takes a reeeallly long time to > > load. > > > > OTOH, if we know that the plugin will never load (e.g. enterprise policy), > then > > yeah, we can notify the page. Tommy, WDYT? > > Hello Bernhard, > > Thanks for reviewing the change.The original bug was raised because no events > were being fired when a plugin could not be loaded.In the added test case , that > is the scenario I have replicated - a mimetype for which a Plugin cannot be > loaded has been added. > > Currently, Firefox does fire an error event, if a plugin for a mimetype cannot > be loaded. Also , it does fire an error event if a plugin is blocked (even if a > plugin can be created for the mimetype). On unblocking the plugin , firefox does > not fire a load event on the object. Okay, being consistent with Firefox is probably a good idea, but I'd like to make sure everyone is okay with that change, as the existing behavior is quite deliberate. +Rachel to confirm (should we loop a PM in as well?) > My debugging so far has revealed that the ChromePluginPlaceholder is > instantiated only when a plugin cannot be created from the existing mimetype > registry, this is so that a plugin object will always be created irrespective of > whether the plugin is supported or not so that a Widget always gets instantiated > (even if the mimetype is not supported) - I am happy to be corrected on this. > > Please note that I could not add code to send load events, because ideally, once > a plugin is loaded I would think that it is the plugins responsibility to fire > events if event handlers are registered.It can be done in the HTMLObjectElement > , but then if a plugin fires its load handler, after a load it would result in > mutilple events for one load.For the purpose of the bug, I think the onus of > dispatching load events should be on the plugin. Unfortunately that's not how things work. Flash (which really is the only plugin at this point) does not fire such an event AFAIK, and I don't think that is likely to change in the future. This is again why I prefer the current behavior (at least for plugins that might load at some point). > > > > Do let me know your thoughts, > Thanks, > George.
On 2017/03/08 18:01:13, Bernhard Bauer wrote: > https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... > File components/plugins/renderer/loadable_plugin_placeholder.cc (right): > > https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... > components/plugins/renderer/loadable_plugin_placeholder.cc:75: > plugin()->markAsPlaceholder(); > On 2017/03/08 14:45:57, George Joseph wrote: > > On 2017/03/08 14:03:14, Bernhard Bauer wrote: > > > If there is a way that the plugin might run eventually (e.g. click-to-play), > > we > > > should _not_ emit an error event. It is by design that the page can't > > > distinguish a blocked plugin from one that just takes a reeeallly long time > to > > > load. > > > > > > OTOH, if we know that the plugin will never load (e.g. enterprise policy), > > then > > > yeah, we can notify the page. Tommy, WDYT? > > > > Hello Bernhard, > > > > Thanks for reviewing the change.The original bug was raised because no events > > were being fired when a plugin could not be loaded.In the added test case , > that > > is the scenario I have replicated - a mimetype for which a Plugin cannot be > > loaded has been added. > > > > Currently, Firefox does fire an error event, if a plugin for a mimetype cannot > > be loaded. Also , it does fire an error event if a plugin is blocked (even if > a > > plugin can be created for the mimetype). On unblocking the plugin , firefox > does > > not fire a load event on the object. > > Okay, being consistent with Firefox is probably a good idea, but I'd like to > make sure everyone is okay with that change, as the existing behavior is quite > deliberate. +Rachel to confirm (should we loop a PM in as well?) > > > My debugging so far has revealed that the ChromePluginPlaceholder is > > instantiated only when a plugin cannot be created from the existing mimetype > > registry, this is so that a plugin object will always be created irrespective > of > > whether the plugin is supported or not so that a Widget always gets > instantiated > > (even if the mimetype is not supported) - I am happy to be corrected on this. > > > > Please note that I could not add code to send load events, because ideally, > once > > a plugin is loaded I would think that it is the plugins responsibility to fire > > events if event handlers are registered.It can be done in the > HTMLObjectElement > > , but then if a plugin fires its load handler, after a load it would result in > > mutilple events for one load.For the purpose of the bug, I think the onus of > > dispatching load events should be on the plugin. > > Unfortunately that's not how things work. Flash (which really is the only plugin > at this point) does not fire such an event AFAIK, and I don't think that is > likely to change in the future. This is again why I prefer the current behavior > (at least for plugins that might load at some point). > > > > > > > > > Do let me know your thoughts, > > Thanks, > > George. And to add a bit more context: ChromePluginPlaceholder is created even if we are just throttling the plugin -- or if it may be loaded later. Emitting an error when there's no plugin matching the mimetype (and nothing will ever be loaded) does indeed seem desirable, esp. if it will match Firefox. Though Flash is the only plugin now -- as bauerb says...
On 2017/03/08 18:05:11, tommycli wrote: > On 2017/03/08 18:01:13, Bernhard Bauer wrote: > > > https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... > > File components/plugins/renderer/loadable_plugin_placeholder.cc (right): > > > > > https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... > > components/plugins/renderer/loadable_plugin_placeholder.cc:75: > > plugin()->markAsPlaceholder(); > > On 2017/03/08 14:45:57, George Joseph wrote: > > > On 2017/03/08 14:03:14, Bernhard Bauer wrote: > > > > If there is a way that the plugin might run eventually (e.g. > click-to-play), > > > we > > > > should _not_ emit an error event. It is by design that the page can't > > > > distinguish a blocked plugin from one that just takes a reeeallly long > time > > to > > > > load. > > > > > > > > OTOH, if we know that the plugin will never load (e.g. enterprise policy), > > > then > > > > yeah, we can notify the page. Tommy, WDYT? > > > > > > Hello Bernhard, > > > > > > Thanks for reviewing the change.The original bug was raised because no > events > > > were being fired when a plugin could not be loaded.In the added test case , > > that > > > is the scenario I have replicated - a mimetype for which a Plugin cannot be > > > loaded has been added. > > > > > > Currently, Firefox does fire an error event, if a plugin for a mimetype > cannot > > > be loaded. Also , it does fire an error event if a plugin is blocked (even > if > > a > > > plugin can be created for the mimetype). On unblocking the plugin , firefox > > does > > > not fire a load event on the object. > > > > Okay, being consistent with Firefox is probably a good idea, but I'd like to > > make sure everyone is okay with that change, as the existing behavior is quite > > deliberate. +Rachel to confirm (should we loop a PM in as well?) > > > > > My debugging so far has revealed that the ChromePluginPlaceholder is > > > instantiated only when a plugin cannot be created from the existing mimetype > > > registry, this is so that a plugin object will always be created > irrespective > > of > > > whether the plugin is supported or not so that a Widget always gets > > instantiated > > > (even if the mimetype is not supported) - I am happy to be corrected on > this. > > > > > > Please note that I could not add code to send load events, because ideally, > > once > > > a plugin is loaded I would think that it is the plugins responsibility to > fire > > > events if event handlers are registered.It can be done in the > > HTMLObjectElement > > > , but then if a plugin fires its load handler, after a load it would result > in > > > mutilple events for one load.For the purpose of the bug, I think the onus of > > > dispatching load events should be on the plugin. > > > > Unfortunately that's not how things work. Flash (which really is the only > plugin > > at this point) does not fire such an event AFAIK, and I don't think that is > > likely to change in the future. This is again why I prefer the current > behavior > > (at least for plugins that might load at some point). > > > > > > > > > > > > > > Do let me know your thoughts, > > > Thanks, > > > George. > > And to add a bit more context: ChromePluginPlaceholder is created even if we are > just throttling the plugin -- or if it may be loaded later. > > Emitting an error when there's no plugin matching the mimetype (and nothing will > ever be loaded) does indeed seem desirable, esp. if it will match Firefox. > Though Flash is the only plugin now -- as bauerb says... Hello Tommycli and Berhhard, Thanks for getting back to me. Just to clarify, in the case of throttling the plugin to be loaded later,Firefox on not loading the plugin does fire the error event despite being capable of rendering the content i,e; if the acrobat plugin is stopped, it does fire the error event. Since this was marked as an interop Issue I implemented a change. Maybe I could add a few checks to see if the plugin is throttled purposely before setting the placeholder flag. Could you give me a scenario where I can replicate throttling the plugin for later loading, so that I can trace the code flow and check the exact states available to loadable_plugin_placeholder.cc to take that decision. From what I have understood today from these conversations,I would think that nonloadable_plugin_placeholder.cc should be loaded when a non loadable plugin object is in the app, however, this does not seem to be created currently. Do let me know your thoughts Thanks, George.
https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... File components/plugins/renderer/loadable_plugin_placeholder.cc (right): https://codereview.chromium.org/2733083004/diff/1/components/plugins/renderer... components/plugins/renderer/loadable_plugin_placeholder.cc:75: plugin()->markAsPlaceholder(); On 2017/03/08 18:01:13, Bernhard Bauer wrote: > On 2017/03/08 14:45:57, George Joseph wrote: > > On 2017/03/08 14:03:14, Bernhard Bauer wrote: > > > If there is a way that the plugin might run eventually (e.g. click-to-play), > > we > > > should _not_ emit an error event. It is by design that the page can't > > > distinguish a blocked plugin from one that just takes a reeeallly long time > to > > > load. > > > > > > OTOH, if we know that the plugin will never load (e.g. enterprise policy), > > then > > > yeah, we can notify the page. Tommy, WDYT? > > > > Hello Bernhard, > > > > Thanks for reviewing the change.The original bug was raised because no events > > were being fired when a plugin could not be loaded.In the added test case , > that > > is the scenario I have replicated - a mimetype for which a Plugin cannot be > > loaded has been added. > > > > Currently, Firefox does fire an error event, if a plugin for a mimetype cannot > > be loaded. Also , it does fire an error event if a plugin is blocked (even if > a > > plugin can be created for the mimetype). On unblocking the plugin , firefox > does > > not fire a load event on the object. > > Okay, being consistent with Firefox is probably a good idea, but I'd like to > make sure everyone is okay with that change, as the existing behavior is quite > deliberate. +Rachel to confirm (should we loop a PM in as well?) > > > My debugging so far has revealed that the ChromePluginPlaceholder is > > instantiated only when a plugin cannot be created from the existing mimetype > > registry, this is so that a plugin object will always be created irrespective > of > > whether the plugin is supported or not so that a Widget always gets > instantiated > > (even if the mimetype is not supported) - I am happy to be corrected on this. > > > > Please note that I could not add code to send load events, because ideally, > once > > a plugin is loaded I would think that it is the plugins responsibility to fire > > events if event handlers are registered.It can be done in the > HTMLObjectElement > > , but then if a plugin fires its load handler, after a load it would result in > > mutilple events for one load.For the purpose of the bug, I think the onus of > > dispatching load events should be on the plugin. > > Unfortunately that's not how things work. Flash (which really is the only plugin > at this point) does not fire such an event AFAIK, and I don't think that is > likely to change in the future. This is again why I prefer the current behavior > (at least for plugins that might load at some point). > > > > > > > > > Do let me know your thoughts, > > Thanks, > > George. > Hello Tommycli and Berhhard, Thanks for getting back to me. Just to clarify, in the case of throttling the plugin to be loaded later,Firefox on not loading the plugin does fire the error event despite being capable of rendering the content i,e; if the acrobat plugin is stopped, it does fire the error event. Since this was marked as an interop Issue I implemented a change. Maybe I could add a few checks to see if the plugin is throttled purposely before setting the placeholder flag. Could you give me a scenario where I can replicate throttling the plugin for later loading, so that I can trace the code flow and check the exact states available to loadable_plugin_placeholder.cc to take that decision. From what I have understood today from these conversations,I would think that nonloadable_plugin_placeholder.cc should be loaded when a non loadable plugin object is in the app, however, this does not seem to be created currently. Do let me know your thoughts Thanks, George.
groby@chromium.org changed reviewers: + laforge@chromium.org
> Okay, being consistent with Firefox is probably a good idea, but I'd like to > make sure everyone is okay with that change, as the existing behavior is quite > deliberate. +Rachel to confirm (should we loop a PM in as well?) Unless there's a web spec that calls for that event, we should probably loop in a PM. (+laforge). IIRC, we deliberately didn't add events indicating load failure. Also, since it's a modification for existing Blink behavior, this probably deserves an Intent to Implement on blink-dev. (Has there been one?)
laforge@chromium.org changed reviewers: + waffles@chromium.org
On 2017/03/13 16:43:30, groby wrote: > > Okay, being consistent with Firefox is probably a good idea, but I'd like to > > make sure everyone is okay with that change, as the existing behavior is quite > > deliberate. +Rachel to confirm (should we loop a PM in as well?) > > Unless there's a web spec that calls for that event, we should probably loop in > a PM. (+laforge). IIRC, we deliberately didn't add events indicating load > failure. > > Also, since it's a modification for existing Blink behavior, this probably > deserves an Intent to Implement on blink-dev. (Has there been one?) +Joshua to comment on the potential implications to silent background fetching/ loading of the Flash Player component (i.e. Chrome doesn't ship w/ Flash Player by default, so we intercept what could be a failed request, fetch the plugin dynamically, and then swap it in once it's installed). If that still works (or rather this CL wouldn't change that behavior), I'm supportive of emitting the correct error events so long as we give public notice (as Rachel suggests).
On 2017/03/13 16:52:38, laforge wrote: > On 2017/03/13 16:43:30, groby wrote: > > > Okay, being consistent with Firefox is probably a good idea, but I'd like to > > > make sure everyone is okay with that change, as the existing behavior is > quite > > > deliberate. +Rachel to confirm (should we loop a PM in as well?) > > > > Unless there's a web spec that calls for that event, we should probably loop > in > > a PM. (+laforge). IIRC, we deliberately didn't add events indicating load > > failure. > > > > Also, since it's a modification for existing Blink behavior, this probably > > deserves an Intent to Implement on blink-dev. (Has there been one?) > > +Joshua to comment on the potential implications to silent background fetching/ > loading of the Flash Player component (i.e. Chrome doesn't ship w/ Flash Player > by default, so we intercept what could be a failed request, fetch the plugin > dynamically, and then swap it in once it's installed). > > If that still works (or rather this CL wouldn't change that behavior), I'm > supportive of emitting the correct error events so long as we give public notice > (as Rachel suggests). Hello Laforge and groby, Thanks for getting back to me. The error event is speced in the object element W3C specification : https://dev.w3.org/html5/spec-preview/the-object-element.html Section 4: If the data attribute is present and its value is not the empty string, then: subsection 3:If that failed, fire a simple event named error at the element, then jump to the last step in the overall set of steps (fallback). subsection 6: If the load failed (e.g. there was an HTTP 404 error, there was a DNS error), fire a simple event named error at the element, then jump to the last step in the overall set of steps (fallback). Currently the fallback content is being displayed, with the error event not being emitted. Thanks, George.
On 2017/03/13 17:59:23, George Joseph wrote: > On 2017/03/13 16:52:38, laforge wrote: > > On 2017/03/13 16:43:30, groby wrote: > > > > Okay, being consistent with Firefox is probably a good idea, but I'd like > to > > > > make sure everyone is okay with that change, as the existing behavior is > > quite > > > > deliberate. +Rachel to confirm (should we loop a PM in as well?) > > > > > > Unless there's a web spec that calls for that event, we should probably loop > > in > > > a PM. (+laforge). IIRC, we deliberately didn't add events indicating load > > > failure. > > > > > > Also, since it's a modification for existing Blink behavior, this probably > > > deserves an Intent to Implement on blink-dev. (Has there been one?) > > > > +Joshua to comment on the potential implications to silent background > fetching/ > > loading of the Flash Player component (i.e. Chrome doesn't ship w/ Flash > Player > > by default, so we intercept what could be a failed request, fetch the plugin > > dynamically, and then swap it in once it's installed). > > > > If that still works (or rather this CL wouldn't change that behavior), I'm > > supportive of emitting the correct error events so long as we give public > notice > > (as Rachel suggests). > Hello Laforge and groby, > Thanks for getting back to me. > > The error event is speced in the object element W3C specification : > > https://dev.w3.org/html5/spec-preview/the-object-element.html > > Section 4: If the data attribute is present and its value is not the empty > string, then: > subsection 3:If that failed, fire a simple event named error at the element, > then jump to the last step in the overall set of steps > (fallback). > > > subsection 6: If the load failed (e.g. there was an HTTP 404 error, there > was a DNS error), fire a simple event named error at the > element, then jump to the last step in the overall set of > steps (fallback). > > > Currently the fallback content is being displayed, with the error event not > being emitted. > > Thanks, > George. George, can you tell me what commit you are synced to so that I can do some manual testing? I am having trouble getting the patch to apply cleanly. (Alternatively, consider syncing to head which will require you to resolve a merge conflict with 0a7e111.)
On 2017/03/13 18:10:05, waffles wrote: > On 2017/03/13 17:59:23, George Joseph wrote: > > On 2017/03/13 16:52:38, laforge wrote: > > > On 2017/03/13 16:43:30, groby wrote: > > > > > Okay, being consistent with Firefox is probably a good idea, but I'd > like > > to > > > > > make sure everyone is okay with that change, as the existing behavior is > > > quite > > > > > deliberate. +Rachel to confirm (should we loop a PM in as well?) > > > > > > > > Unless there's a web spec that calls for that event, we should probably > loop > > > in > > > > a PM. (+laforge). IIRC, we deliberately didn't add events indicating load > > > > failure. > > > > > > > > Also, since it's a modification for existing Blink behavior, this probably > > > > deserves an Intent to Implement on blink-dev. (Has there been one?) > > > > > > +Joshua to comment on the potential implications to silent background > > fetching/ > > > loading of the Flash Player component (i.e. Chrome doesn't ship w/ Flash > > Player > > > by default, so we intercept what could be a failed request, fetch the plugin > > > dynamically, and then swap it in once it's installed). > > > > > > If that still works (or rather this CL wouldn't change that behavior), I'm > > > supportive of emitting the correct error events so long as we give public > > notice > > > (as Rachel suggests). > > Hello Laforge and groby, > > Thanks for getting back to me. > > > > The error event is speced in the object element W3C specification : > > > > https://dev.w3.org/html5/spec-preview/the-object-element.html > > > > Section 4: If the data attribute is present and its value is not the empty > > string, then: > > subsection 3:If that failed, fire a simple event named error at the > element, > > then jump to the last step in the overall set of steps > > (fallback). > > > > > > subsection 6: If the load failed (e.g. there was an HTTP 404 error, there > > was a DNS error), fire a simple event named error at the > > element, then jump to the last step in the overall set of > > steps (fallback). > > > > > > Currently the fallback content is being displayed, with the error event not > > being emitted. > > > > Thanks, > > George. > > George, can you tell me what commit you are synced to so that I can do some > manual testing? I am having trouble getting the patch to apply cleanly. > (Alternatively, consider syncing to head which will require you to resolve a > merge conflict with 0a7e111.) Hello Waffles, I have uploaded a patchset after reconciling to head. .The additional test added passes however one seems to fail, though the content test does not report which one failed. Do let me know your thoughts. Thanks, George.
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/14 06:19:08, George Joseph wrote: > On 2017/03/13 18:10:05, waffles wrote: > > On 2017/03/13 17:59:23, George Joseph wrote: > > > On 2017/03/13 16:52:38, laforge wrote: > > > > On 2017/03/13 16:43:30, groby wrote: > > > > > > Okay, being consistent with Firefox is probably a good idea, but I'd > > like > > > to > > > > > > make sure everyone is okay with that change, as the existing behavior > is > > > > quite > > > > > > deliberate. +Rachel to confirm (should we loop a PM in as well?) > > > > > > > > > > Unless there's a web spec that calls for that event, we should probably > > loop > > > > in > > > > > a PM. (+laforge). IIRC, we deliberately didn't add events indicating > load > > > > > failure. > > > > > > > > > > Also, since it's a modification for existing Blink behavior, this > probably > > > > > deserves an Intent to Implement on blink-dev. (Has there been one?) > > > > > > > > +Joshua to comment on the potential implications to silent background > > > fetching/ > > > > loading of the Flash Player component (i.e. Chrome doesn't ship w/ Flash > > > Player > > > > by default, so we intercept what could be a failed request, fetch the > plugin > > > > dynamically, and then swap it in once it's installed). > > > > > > > > If that still works (or rather this CL wouldn't change that behavior), I'm > > > > supportive of emitting the correct error events so long as we give public > > > notice > > > > (as Rachel suggests). > > > Hello Laforge and groby, > > > Thanks for getting back to me. > > > > > > The error event is speced in the object element W3C specification : > > > > > > https://dev.w3.org/html5/spec-preview/the-object-element.html > > > > > > Section 4: If the data attribute is present and its value is not the empty > > > string, then: > > > subsection 3:If that failed, fire a simple event named error at the > > element, > > > then jump to the last step in the overall set of steps > > > (fallback). > > > > > > > > > subsection 6: If the load failed (e.g. there was an HTTP 404 error, > there > > > was a DNS error), fire a simple event named error at the > > > element, then jump to the last step in the overall set of > > > steps (fallback). > > > > > > > > > Currently the fallback content is being displayed, with the error event not > > > being emitted. > > > > > > Thanks, > > > George. > > > > George, can you tell me what commit you are synced to so that I can do some > > manual testing? I am having trouble getting the patch to apply cleanly. > > (Alternatively, consider syncing to head which will require you to resolve a > > merge conflict with 0a7e111.) > > Hello Waffles, > I have uploaded a patchset after reconciling to head. .The additional test > added passes however one seems to fail, though the content test does not report > which one > failed. > > Do let me know your thoughts. > Thanks, > George. Updated the test with the correct </object> termination character.
On 2017/03/14 23:53:14, George Joseph wrote: > On 2017/03/14 06:19:08, George Joseph wrote: > > On 2017/03/13 18:10:05, waffles wrote: > > > On 2017/03/13 17:59:23, George Joseph wrote: > > > > On 2017/03/13 16:52:38, laforge wrote: > > > > > On 2017/03/13 16:43:30, groby wrote: > > > > > > > Okay, being consistent with Firefox is probably a good idea, but I'd > > > like > > > > to > > > > > > > make sure everyone is okay with that change, as the existing > behavior > > is > > > > > quite > > > > > > > deliberate. +Rachel to confirm (should we loop a PM in as well?) > > > > > > > > > > > > Unless there's a web spec that calls for that event, we should > probably > > > loop > > > > > in > > > > > > a PM. (+laforge). IIRC, we deliberately didn't add events indicating > > load > > > > > > failure. > > > > > > > > > > > > Also, since it's a modification for existing Blink behavior, this > > probably > > > > > > deserves an Intent to Implement on blink-dev. (Has there been one?) > > > > > > > > > > +Joshua to comment on the potential implications to silent background > > > > fetching/ > > > > > loading of the Flash Player component (i.e. Chrome doesn't ship w/ Flash > > > > Player > > > > > by default, so we intercept what could be a failed request, fetch the > > plugin > > > > > dynamically, and then swap it in once it's installed). > > > > > > > > > > If that still works (or rather this CL wouldn't change that behavior), > I'm > > > > > supportive of emitting the correct error events so long as we give > public > > > > notice > > > > > (as Rachel suggests). > > > > Hello Laforge and groby, > > > > Thanks for getting back to me. > > > > > > > > The error event is speced in the object element W3C specification : > > > > > > > > https://dev.w3.org/html5/spec-preview/the-object-element.html > > > > > > > > Section 4: If the data attribute is present and its value is not the empty > > > > string, then: > > > > subsection 3:If that failed, fire a simple event named error at the > > > element, > > > > then jump to the last step in the overall set of steps > > > > (fallback). > > > > > > > > > > > > subsection 6: If the load failed (e.g. there was an HTTP 404 error, > > there > > > > was a DNS error), fire a simple event named error at the > > > > element, then jump to the last step in the overall set > of > > > > steps (fallback). > > > > > > > > > > > > Currently the fallback content is being displayed, with the error event > not > > > > being emitted. > > > > > > > > Thanks, > > > > George. > > > > > > George, can you tell me what commit you are synced to so that I can do some > > > manual testing? I am having trouble getting the patch to apply cleanly. > > > (Alternatively, consider syncing to head which will require you to resolve a > > > merge conflict with 0a7e111.) > > > > Hello Waffles, > > I have uploaded a patchset after reconciling to head. .The additional test > > added passes however one seems to fail, though the content test does not > report > > which one > > failed. > > > > Do let me know your thoughts. > > Thanks, > > George. > > Updated the test with the correct </object> termination character. Hey George, does Chrome currently ever fire an error event?
On 2017/03/15 00:29:06, tommycli wrote: > On 2017/03/14 23:53:14, George Joseph wrote: > > On 2017/03/14 06:19:08, George Joseph wrote: > > > On 2017/03/13 18:10:05, waffles wrote: > > > > On 2017/03/13 17:59:23, George Joseph wrote: > > > > > On 2017/03/13 16:52:38, laforge wrote: > > > > > > On 2017/03/13 16:43:30, groby wrote: > > > > > > > > Okay, being consistent with Firefox is probably a good idea, but > I'd > > > > like > > > > > to > > > > > > > > make sure everyone is okay with that change, as the existing > > behavior > > > is > > > > > > quite > > > > > > > > deliberate. +Rachel to confirm (should we loop a PM in as well?) > > > > > > > > > > > > > > Unless there's a web spec that calls for that event, we should > > probably > > > > loop > > > > > > in > > > > > > > a PM. (+laforge). IIRC, we deliberately didn't add events indicating > > > load > > > > > > > failure. > > > > > > > > > > > > > > Also, since it's a modification for existing Blink behavior, this > > > probably > > > > > > > deserves an Intent to Implement on blink-dev. (Has there been one?) > > > > > > > > > > > > +Joshua to comment on the potential implications to silent background > > > > > fetching/ > > > > > > loading of the Flash Player component (i.e. Chrome doesn't ship w/ > Flash > > > > > Player > > > > > > by default, so we intercept what could be a failed request, fetch the > > > plugin > > > > > > dynamically, and then swap it in once it's installed). > > > > > > > > > > > > If that still works (or rather this CL wouldn't change that behavior), > > I'm > > > > > > supportive of emitting the correct error events so long as we give > > public > > > > > notice > > > > > > (as Rachel suggests). > > > > > Hello Laforge and groby, > > > > > Thanks for getting back to me. > > > > > > > > > > The error event is speced in the object element W3C specification : > > > > > > > > > > https://dev.w3.org/html5/spec-preview/the-object-element.html > > > > > > > > > > Section 4: If the data attribute is present and its value is not the > empty > > > > > string, then: > > > > > subsection 3:If that failed, fire a simple event named error at the > > > > element, > > > > > then jump to the last step in the overall set of steps > > > > > (fallback). > > > > > > > > > > > > > > > subsection 6: If the load failed (e.g. there was an HTTP 404 error, > > > there > > > > > was a DNS error), fire a simple event named error at the > > > > > element, then jump to the last step in the overall > set > > of > > > > > steps (fallback). > > > > > > > > > > > > > > > Currently the fallback content is being displayed, with the error event > > not > > > > > being emitted. > > > > > > > > > > Thanks, > > > > > George. > > > > > > > > George, can you tell me what commit you are synced to so that I can do > some > > > > manual testing? I am having trouble getting the patch to apply cleanly. > > > > (Alternatively, consider syncing to head which will require you to resolve > a > > > > merge conflict with 0a7e111.) > > > > > > Hello Waffles, > > > I have uploaded a patchset after reconciling to head. .The additional test > > > added passes however one seems to fail, though the content test does not > > report > > > which one > > > failed. > > > > > > Do let me know your thoughts. > > > Thanks, > > > George. > > > > Updated the test with the correct </object> termination character. > > Hey George, does Chrome currently ever fire an error event? Okay, I answered my own question. Oops. So... reading through this thread more carefully, I think there is merit to this patch overall. In George's provided test case: <object type="image/testabcd" data="hello" id="object1" onerror="errorHandler()"></object>, we should definitely fire the error event if it's specified by the spec (and Firefox does). If Chrome doesn't currently fire an error event, we should fix it. We just need to be careful for cases where the plugin is throttled but may be loaded later. To create an example: 1. Set your Flash setting under chrome://settings/content to 'Allow sites to run Flash'. 2. Load http://pps-test-a.appspot.com/small_only.html 3. You should see a screen like this: http://imgur.com/a/DtgTJ In those cases, I think Chrome should NOT fire an error event, because there's not really an error... it should appear to the page that the plugin is just loading infinitely slowly. In the code, if LoadablePluginPlaceholder has AllowLoading() == true, we probably should not fire an error event, since the plugin may load later.
On 2017/03/15 00:45:47, tommycli wrote: > On 2017/03/15 00:29:06, tommycli wrote: > > On 2017/03/14 23:53:14, George Joseph wrote: > > > On 2017/03/14 06:19:08, George Joseph wrote: > > > > On 2017/03/13 18:10:05, waffles wrote: > > > > > On 2017/03/13 17:59:23, George Joseph wrote: > > > > > > On 2017/03/13 16:52:38, laforge wrote: > > > > > > > On 2017/03/13 16:43:30, groby wrote: > > > > > > > > > Okay, being consistent with Firefox is probably a good idea, but > > I'd > > > > > like > > > > > > to > > > > > > > > > make sure everyone is okay with that change, as the existing > > > behavior > > > > is > > > > > > > quite > > > > > > > > > deliberate. +Rachel to confirm (should we loop a PM in as well?) > > > > > > > > > > > > > > > > Unless there's a web spec that calls for that event, we should > > > probably > > > > > loop > > > > > > > in > > > > > > > > a PM. (+laforge). IIRC, we deliberately didn't add events > indicating > > > > load > > > > > > > > failure. > > > > > > > > > > > > > > > > Also, since it's a modification for existing Blink behavior, this > > > > probably > > > > > > > > deserves an Intent to Implement on blink-dev. (Has there been > one?) > > > > > > > > > > > > > > +Joshua to comment on the potential implications to silent > background > > > > > > fetching/ > > > > > > > loading of the Flash Player component (i.e. Chrome doesn't ship w/ > > Flash > > > > > > Player > > > > > > > by default, so we intercept what could be a failed request, fetch > the > > > > plugin > > > > > > > dynamically, and then swap it in once it's installed). > > > > > > > > > > > > > > If that still works (or rather this CL wouldn't change that > behavior), > > > I'm > > > > > > > supportive of emitting the correct error events so long as we give > > > public > > > > > > notice > > > > > > > (as Rachel suggests). > > > > > > Hello Laforge and groby, > > > > > > Thanks for getting back to me. > > > > > > > > > > > > The error event is speced in the object element W3C specification : > > > > > > > > > > > > https://dev.w3.org/html5/spec-preview/the-object-element.html > > > > > > > > > > > > Section 4: If the data attribute is present and its value is not the > > empty > > > > > > string, then: > > > > > > subsection 3:If that failed, fire a simple event named error at > the > > > > > element, > > > > > > then jump to the last step in the overall set of steps > > > > > > (fallback). > > > > > > > > > > > > > > > > > > subsection 6: If the load failed (e.g. there was an HTTP 404 > error, > > > > there > > > > > > was a DNS error), fire a simple event named error at the > > > > > > element, then jump to the last step in the overall > > set > > > of > > > > > > steps (fallback). > > > > > > > > > > > > > > > > > > Currently the fallback content is being displayed, with the error > event > > > not > > > > > > being emitted. > > > > > > > > > > > > Thanks, > > > > > > George. > > > > > > > > > > George, can you tell me what commit you are synced to so that I can do > > some > > > > > manual testing? I am having trouble getting the patch to apply cleanly. > > > > > (Alternatively, consider syncing to head which will require you to > resolve > > a > > > > > merge conflict with 0a7e111.) > > > > > > > > Hello Waffles, > > > > I have uploaded a patchset after reconciling to head. .The additional test > > > > > added passes however one seems to fail, though the content test does not > > > report > > > > which one > > > > failed. > > > > > > > > Do let me know your thoughts. > > > > Thanks, > > > > George. > > > > > > Updated the test with the correct </object> termination character. > > > > Hey George, does Chrome currently ever fire an error event? > > Okay, I answered my own question. Oops. > > So... reading through this thread more carefully, I think there is merit to this > patch overall. > > In George's provided test case: <object type="image/testabcd" data="hello" > id="object1" onerror="errorHandler()"></object>, we should definitely fire the > error event if it's specified by the spec (and Firefox does). > > If Chrome doesn't currently fire an error event, we should fix it. > > We just need to be careful for cases where the plugin is throttled but may be > loaded later. To create an example: > 1. Set your Flash setting under chrome://settings/content to 'Allow sites to > run Flash'. > 2. Load http://pps-test-a.appspot.com/small_only.html > 3. You should see a screen like this: http://imgur.com/a/DtgTJ > > In those cases, I think Chrome should NOT fire an error event, because there's > not really an error... it should appear to the page that the plugin is just > loading infinitely slowly. > > In the code, if LoadablePluginPlaceholder has AllowLoading() == true, we > probably should not fire an error event, since the plugin may load later. To be clear: what i mean is, throttled plugins should always have AllowLoading() == true, so if you don't throw an error in those cases, you should be covered. bauerb: Please check my logic, since we're kind of in new territory here.
https://codereview.chromium.org/2733083004/diff/40001/components/plugins/rend... File components/plugins/renderer/loadable_plugin_placeholder.cc (right): https://codereview.chromium.org/2733083004/diff/40001/components/plugins/rend... components/plugins/renderer/loadable_plugin_placeholder.cc:75: plugin()->markAsPlaceholder(); Since some placeholders are temporary, and others are truly "error" placeholders, I think this method should be renamed... maybe markAsErrorPlaceholder? Also I think this method should only be called if AllowLoading is false. This is tricky because currently AllowLoading is called separately (see https://cs.chromium.org/chromium/src/chrome/renderer/chrome_content_renderer_...) from the constructor. It probably needs to be merged into the constructor call. I think to cover Android, NonLoadablePluginPlaceholder should also call this method. That one should always call it since it's never loadable.
On 2017/03/15 00:58:29, tommycli wrote: > https://codereview.chromium.org/2733083004/diff/40001/components/plugins/rend... > File components/plugins/renderer/loadable_plugin_placeholder.cc (right): > > https://codereview.chromium.org/2733083004/diff/40001/components/plugins/rend... > components/plugins/renderer/loadable_plugin_placeholder.cc:75: > plugin()->markAsPlaceholder(); > Since some placeholders are temporary, and others are truly "error" > placeholders, I think this method should be renamed... > > maybe markAsErrorPlaceholder? Also I think this method should only be called if > AllowLoading is false. This is tricky because currently AllowLoading is called > separately (see > https://cs.chromium.org/chromium/src/chrome/renderer/chrome_content_renderer_...) > from the constructor. It probably needs to be merged into the constructor call. > > I think to cover Android, NonLoadablePluginPlaceholder should also call this > method. That one should always call it since it's never loadable. Hello Tommycli, Thanks for the review comments. I will try to find a clean way to integrate the change. I will add the hooks to NonLoadablePluginPlaceholder as well. Currently, chromium component builds dont work well on xenial so I have to create a static link, which takes a huge amount of time on my machine. Hopefully I can land the change in the next couple of days. Thanks, George.
Based on my testing patch set 3 does not interfere with JIT installation of Flash. This resolves laforge's concern from comment #25. If the owners lgtm sometime next week I will do a final test on the final patchset. (Else, I will be on leave.)
On 2017/03/17 23:19:49, waffles wrote: > Based on my testing patch set 3 does not interfere with JIT installation of > Flash. This resolves laforge's concern from comment #25. > > If the owners lgtm sometime next week I will do a final test on the final > patchset. (Else, I will be on leave.) not lgtm I meant to say "if the owners l-g-t-m", sorry.
On 2017/03/17 23:20:42, waffles wrote: > On 2017/03/17 23:19:49, waffles wrote: > > Based on my testing patch set 3 does not interfere with JIT installation of > > Flash. This resolves laforge's concern from comment #25. > > > > If the owners lgtm sometime next week I will do a final test on the final > > patchset. (Else, I will be on leave.) > > not lgtm > > I meant to say "if the owners l-g-t-m", sorry. Hello Tommycli and Waffles, Thanks for getting back to me.Apologies for the delay. I have uploaded the changes based on your comments. Please do let me know if this meets the expectations as mentioned in the comments. Thanks, George
https://codereview.chromium.org/2733083004/diff/40001/components/plugins/rend... File components/plugins/renderer/loadable_plugin_placeholder.cc (right): https://codereview.chromium.org/2733083004/diff/40001/components/plugins/rend... components/plugins/renderer/loadable_plugin_placeholder.cc:75: plugin()->markAsPlaceholder(); On 2017/03/15 00:58:29, tommycli wrote: > Since some placeholders are temporary, and others are truly "error" > placeholders, I think this method should be renamed... > > maybe markAsErrorPlaceholder? Also I think this method should only be called if > AllowLoading is false. This is tricky because currently AllowLoading is called > separately (see > https://cs.chromium.org/chromium/src/chrome/renderer/chrome_content_renderer_...) > from the constructor. It probably needs to be merged into the constructor call. > > I think to cover Android, NonLoadablePluginPlaceholder should also call this > method. That one should always call it since it's never loadable. Done.
On 2017/03/22 00:04:09, George Joseph wrote: > https://codereview.chromium.org/2733083004/diff/40001/components/plugins/rend... > File components/plugins/renderer/loadable_plugin_placeholder.cc (right): > > https://codereview.chromium.org/2733083004/diff/40001/components/plugins/rend... > components/plugins/renderer/loadable_plugin_placeholder.cc:75: > plugin()->markAsPlaceholder(); > On 2017/03/15 00:58:29, tommycli wrote: > > Since some placeholders are temporary, and others are truly "error" > > placeholders, I think this method should be renamed... > > > > maybe markAsErrorPlaceholder? Also I think this method should only be called > if > > AllowLoading is false. This is tricky because currently AllowLoading is called > > separately (see > > > https://cs.chromium.org/chromium/src/chrome/renderer/chrome_content_renderer_...) > > from the constructor. It probably needs to be merged into the constructor > call. > > > > I think to cover Android, NonLoadablePluginPlaceholder should also call this > > method. That one should always call it since it's never loadable. > > Done. Hi, Any possibility of a review this week. Thanks, George.
https://codereview.chromium.org/2733083004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:7: var testobject = async_test("crbug.com/445557 : error events aren't dispatched for <object> elements"); Nit: Try keeping everything in 80 columns. https://codereview.chromium.org/2733083004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:9: { Nit: Brace on previous line. https://codereview.chromium.org/2733083004/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebPlugin.h (right): https://codereview.chromium.org/2733083004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebPlugin.h:246: bool placeholder = false; WebPlugin is an interface; it shouldn't have members (and only trivial method implementations). This should be on WebViewPlugin.
kottackal.george@gmail.com changed reviewers: + sorin@google.com
Hello Bernhard, Thanks for reviewing the changes I made. I have uploaded patchset 5 with the recommended changes. Do let me know your thoughts. Thanks, George. https://codereview.chromium.org/2733083004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:7: var testobject = async_test("crbug.com/445557 : error events aren't dispatched for <object> elements"); On 2017/03/27 09:20:27, Bernhard (OOO until Mar 27) wrote: > Nit: Try keeping everything in 80 columns. Done. https://codereview.chromium.org/2733083004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:9: { On 2017/03/27 09:20:27, Bernhard (OOO until Mar 27) wrote: > Nit: Brace on previous line. Done. https://codereview.chromium.org/2733083004/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebPlugin.h (right): https://codereview.chromium.org/2733083004/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebPlugin.h:246: bool placeholder = false; On 2017/03/27 09:20:27, Bernhard (OOO until Mar 27) wrote: > WebPlugin is an interface; it shouldn't have members (and only trivial method > implementations). This should be on WebViewPlugin. Done.
Hello Bernhard, Thanks for reviewing the changes I made. I have uploaded patchset 5 with the recommended changes. Do let me know your thoughts. Thanks, George.
https://codereview.chromium.org/2733083004/diff/80001/components/plugins/rend... File components/plugins/renderer/webview_plugin.h (right): https://codereview.chromium.org/2733083004/diff/80001/components/plugins/rend... components/plugins/renderer/webview_plugin.h:109: void markAsErrorPlaceholder(); These two methods are not overrides of WebPlugin, so they should not be grouped with isPlaceholder(). Coming to think of it, whether or not this is an error placeholder is really defined by the client of this class, so instead of using a flag in here we could make this a delegate method. PluginPlaceholderBase would implement the delegate method to return false, and LoadablePluginPlaceholder would return !allow_loading_. https://codereview.chromium.org/2733083004/diff/80001/components/plugins/rend... components/plugins/renderer/webview_plugin.h:144: bool placeholder = false; Member variables in Chromium style end with an underscore. (Although we can remove this member, see above.) https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:8: var testobject = async_test("crbug.com/445557:error events aren't dispatched"+ Nit: Spaces around the plus sign (which means you have to break after "aren't". https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:195: return false; I would maybe return early in this case instead. https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebPlugin.h (right): https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebPlugin.h:240: virtual bool isPlaceholder() { return false; } I don't think you need to change the default implementation now. All the cases we're interested in derive from WebViewPlugin anyway, right?
Hello Bernhard, Thanks for the review comments. I have made changes as proposed and verified the test cases again. Please do let me know if the changes meet your expectations. Thanks, George. https://codereview.chromium.org/2733083004/diff/80001/components/plugins/rend... File components/plugins/renderer/webview_plugin.h (right): https://codereview.chromium.org/2733083004/diff/80001/components/plugins/rend... components/plugins/renderer/webview_plugin.h:109: void markAsErrorPlaceholder(); On 2017/03/28 10:55:59, Bernhard Bauer wrote: > These two methods are not overrides of WebPlugin, so they should not be grouped > with isPlaceholder(). > > Coming to think of it, whether or not this is an error placeholder is really > defined by the client of this class, so instead of using a flag in here we could > make this a delegate method. PluginPlaceholderBase would implement the delegate > method to return false, and LoadablePluginPlaceholder would return > !allow_loading_. Done. https://codereview.chromium.org/2733083004/diff/80001/components/plugins/rend... components/plugins/renderer/webview_plugin.h:144: bool placeholder = false; On 2017/03/28 10:55:59, Bernhard Bauer wrote: > Member variables in Chromium style end with an underscore. (Although we can > remove this member, see above.) Done. https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:8: var testobject = async_test("crbug.com/445557:error events aren't dispatched"+ On 2017/03/28 10:55:59, Bernhard Bauer wrote: > Nit: Spaces around the plus sign (which means you have to break after "aren't". Done. https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebPluginContainerImpl.cpp (right): https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebPluginContainerImpl.cpp:195: return false; On 2017/03/28 10:55:59, Bernhard Bauer wrote: > I would maybe return early in this case instead. Done. https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebPlugin.h (right): https://codereview.chromium.org/2733083004/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebPlugin.h:240: virtual bool isPlaceholder() { return false; } On 2017/03/28 10:55:59, Bernhard Bauer wrote: > I don't think you need to change the default implementation now. All the cases > we're interested in derive from WebViewPlugin anyway, right? Done.
Mostly nits now. https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... File components/plugins/renderer/loadable_plugin_placeholder.cc (right): https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... components/plugins/renderer/loadable_plugin_placeholder.cc:183: } Nit: empty line after this one. https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... File components/plugins/renderer/loadable_plugin_placeholder.h (right): https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... components/plugins/renderer/loadable_plugin_placeholder.h:94: Nit: leave out the empty line here; the whole block of methods is WebViewPlugin::Delegate overrides. https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... File components/plugins/renderer/webview_plugin.h (right): https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... components/plugins/renderer/webview_plugin.h:59: virtual bool isErrorPlaceholder() = 0; This is Chrome code, so method names should be start with an uppercase letter. https://codereview.chromium.org/2733083004/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebPlugin.h (right): https://codereview.chromium.org/2733083004/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:136: virtual bool isErrorPlaceholder() { return false; }; Move this down, maybe to isPlaceholder()? The method up here have little to do with it. And then maybe add a comment, in particular explaining the difference between isPlaceholder() and isErrorPlaceholder()?
Hello Bernhard, Thanks for the reviewing my change. I have made changes as mentioned in the comment. Please do review and let me know if this meets the expectations as per the review comments. Thanks, George. https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... File components/plugins/renderer/loadable_plugin_placeholder.cc (right): https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... components/plugins/renderer/loadable_plugin_placeholder.cc:183: } On 2017/03/30 09:08:28, Bernhard Bauer wrote: > Nit: empty line after this one. Done. https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... File components/plugins/renderer/loadable_plugin_placeholder.h (right): https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... components/plugins/renderer/loadable_plugin_placeholder.h:94: On 2017/03/30 09:08:28, Bernhard Bauer wrote: > Nit: leave out the empty line here; the whole block of methods is > WebViewPlugin::Delegate overrides. Done. https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... File components/plugins/renderer/webview_plugin.h (right): https://codereview.chromium.org/2733083004/diff/100001/components/plugins/ren... components/plugins/renderer/webview_plugin.h:59: virtual bool isErrorPlaceholder() = 0; On 2017/03/30 09:08:28, Bernhard Bauer wrote: > This is Chrome code, so method names should be start with an uppercase letter. Done.
https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:6: onerror="errorHandler()"></object> This line should be indented four spaces (similar to C++ code; it's a broken continuation from the previous line). I think you can also break right before the closing </object> tag. https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:8: var testobject = async_test("crbug.com/445557:error events" This seems like it's shorter than 80 characters now. The general rule is fit everything into 80 characters, so I think you can do: var testobject = async_test("crbug.com/445557:error events aren't dispatched" + " for <object> elements"); https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebPlugin.h (right): https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:242: // PluginPlaceholderBase derived plugin, which is not allowed This is a layering violation, as Blink shouldn't know about Chrome code. https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:243: // to load later on. isPlaceholder() is used by the pepper This sentence should be a comment on isPlaceholder, but it also violates the layering. It also doesn't actually convey much more information than is already in the name. AFAIU, the big difference is that isPlaceholder() returns whether the plugin can't be interacted with _right now_ but might load later, whereas isErrorPlaceholder() returns whether the plugin will _never_ load (and therefore we should emit the error event). https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:245: // did create a PlaceHolder Plugin. Nit: capitalized weirdly (placeholder should be a single lowercased word, as should plugin). https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:246: virtual bool isErrorPlaceholder() { return false; }; Nit: No semicolon after the closing brace.
Hello Bernhard, Thanks for the review comments. Thanks for being patient with me. I have uploaded the cl with the changes as mentioned in the review. Do let me know your thoughts. Thanks, George. https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:6: onerror="errorHandler()"></object> On 2017/03/31 09:25:20, Bernhard Bauer wrote: > This line should be indented four spaces (similar to C++ code; it's a broken > continuation from the previous line). I think you can also break right before > the closing </object> tag. Done. https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:8: var testobject = async_test("crbug.com/445557:error events" On 2017/03/31 09:25:20, Bernhard Bauer wrote: > This seems like it's shorter than 80 characters now. The general rule is fit > everything into 80 characters, so I think you can do: > > var testobject = async_test("crbug.com/445557:error events aren't dispatched" + > " for <object> elements"); Done. https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebPlugin.h (right): https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:242: // PluginPlaceholderBase derived plugin, which is not allowed On 2017/03/31 09:25:20, Bernhard Bauer wrote: > This is a layering violation, as Blink shouldn't know about Chrome code. Done. https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:245: // did create a PlaceHolder Plugin. On 2017/03/31 09:25:20, Bernhard (slow until Apr 6) wrote: > Nit: capitalized weirdly (placeholder should be a single lowercased word, as > should plugin). Done. https://codereview.chromium.org/2733083004/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:246: virtual bool isErrorPlaceholder() { return false; }; On 2017/03/31 09:25:21, Bernhard (slow until Apr 6) wrote: > Nit: No semicolon after the closing brace. Done.
Almost there :) I would also start looking for a Blink reviewer, as I'm not in the OWNERS file there. https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:9: " for <object> elements"); Nit: Subsequent lines are indented by four characters more. https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebPlugin.h (right): https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:239: // Check whether a plugin can be interacted with.A positive return value Nit: Space after period. https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:241: // The plugin could ,however, load successfully later. Nit: swap comma and space (the first instance plz 😉) https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:243: // Check whether a plugin failed to load, with there being no possibility of Nit: collapse two spaces between "being" and "no" into one. https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:245: virtual bool isErrorplaceholder() { return false; } Nit: isErrorPlaceholder (the CamelCase version of "is error placeholder"), and in the WebViewPlugin::Delegate method IsErrorPlaceholder.
hey, overall with Bernhard's suggestions, I'm satisfied with these changes. lgtm. But like he said you will need a Blink side reviewer. https://codereview.chromium.org/2733083004/diff/140001/components/plugins/ren... File components/plugins/renderer/webview_plugin.h (right): https://codereview.chromium.org/2733083004/diff/140001/components/plugins/ren... components/plugins/renderer/webview_plugin.h:59: virtual bool IsErrorplaceholder() = 0; Yes, to echo bauerb, this should be IsErrorPlaceholder() https://codereview.chromium.org/2733083004/diff/140001/components/plugins/ren... components/plugins/renderer/webview_plugin.h:89: bool isErrorplaceholder() override; and isErrorPlaceholder here
Hello Bernhard and Tommycli, Thanks for the review comments and the LGTM. I have uploaded the changes as per the review comments.Any ideas of whom I could add for review on the Blink side.. Thanks, George. https://codereview.chromium.org/2733083004/diff/140001/components/plugins/ren... File components/plugins/renderer/webview_plugin.h (right): https://codereview.chromium.org/2733083004/diff/140001/components/plugins/ren... components/plugins/renderer/webview_plugin.h:59: virtual bool IsErrorplaceholder() = 0; On 2017/04/03 22:15:49, tommycli wrote: > Yes, to echo bauerb, this should be IsErrorPlaceholder() Done. https://codereview.chromium.org/2733083004/diff/140001/components/plugins/ren... components/plugins/renderer/webview_plugin.h:89: bool isErrorplaceholder() override; On 2017/04/03 22:15:49, tommycli wrote: > and isErrorPlaceholder here Done. https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:9: " for <object> elements"); On 2017/04/03 10:14:20, Bernhard (slow until Apr 6) wrote: > Nit: Subsequent lines are indented by four characters more. Done. https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebPlugin.h (right): https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:239: // Check whether a plugin can be interacted with.A positive return value On 2017/04/03 10:14:20, Bernhard (slow until Apr 6) wrote: > Nit: Space after period. Done. https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:241: // The plugin could ,however, load successfully later. On 2017/04/03 10:14:20, Bernhard (slow until Apr 6) wrote: > Nit: swap comma and space (the first instance plz 😉) Done. https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:243: // Check whether a plugin failed to load, with there being no possibility of On 2017/04/03 10:14:20, Bernhard (slow until Apr 6) wrote: > Nit: collapse two spaces between "being" and "no" into one. Done. https://codereview.chromium.org/2733083004/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebPlugin.h:245: virtual bool isErrorplaceholder() { return false; } On 2017/04/03 10:14:20, Bernhard (slow until Apr 6) wrote: > Nit: isErrorPlaceholder (the CamelCase version of "is error placeholder"), and > in the WebViewPlugin::Delegate method IsErrorPlaceholder. Done.
bauerb@chromium.org changed reviewers: + jochen@chromium.org
Maybe +jochen could review the Blink side? It's also a bit problematic that there's still an n-o-l-g-t-m from Joshua on the CL, but he's still OOO for a bit. Sorin, if you approve, can we move Joshua to CC (assuming that works?)? https://codereview.chromium.org/2733083004/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:9: " for <object> elements"); _Four_ characters please :) (Right now it's three characters indented.) https://codereview.chromium.org/2733083004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2733083004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:432: if (pluginWidget() && (pluginWidget()->isPluginContainer()) && Nit: Parentheses around the individual expressions aren't necessary (`pluginWidget() && pluginWidget()->isPluginContainer() && pluginWidget()->isErrorplaceholder()` works).
is it possible to also test that error events aren't dispatched for non-error placeholders?
On 2017/04/05 11:01:37, jochen wrote: > is it possible to also test that error events aren't dispatched for non-error > placeholders? Is there anything specific you'd like me to review? (This looks like a huge set of reviewers for this change. Tommy knows components/plugins better than I do, and if there's no specific feedback you need from me, please feel free to remove me)
kottackal.george@gmail.com changed reviewers: - groby@chromium.org
-Rachel. Hello Bernhard and Jochen, Thanks for the review comments. I have made the changes as suggested, I have added a test to check for onerror events in the case when a load is successful. Do let me know your thoughts. Thanks, George. https://codereview.chromium.org/2733083004/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:9: " for <object> elements"); On 2017/04/05 09:56:20, Bernhard Bauer wrote: > _Four_ characters please :) (Right now it's three characters indented.) Done. https://codereview.chromium.org/2733083004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2733083004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:432: if (pluginWidget() && (pluginWidget()->isPluginContainer()) && On 2017/04/05 09:56:20, Bernhard Bauer wrote: > Nit: Parentheses around the individual expressions aren't necessary > (`pluginWidget() && pluginWidget()->isPluginContainer() && > pluginWidget()->isErrorplaceholder()` works). Done.
-Rachel. Hello Bernhard and Jochen, Thanks for the review comments. I have made the changes as suggested, I have added a test to check for onerror events in the case when a load is successful. Do let me know your thoughts. Thanks, George.
https://codereview.chromium.org/2733083004/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:7: style="display:none" ></object> This is formatted pretty badly. Please familiarize yourself with the Web style guide at https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md and format your code accordingly. https://codereview.chromium.org/2733083004/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:13: document.getElementById("objectimage").onerror = imageErrorHandler; I don't think you need to chain these two tests together. Maybe make two separate tests? https://codereview.chromium.org/2733083004/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:15: testobject.step_timeout(function() { Using timeouts isn't ideal (sometimes tests can legitimately take longer, leading to flakiness, or the timeout can be too long, leading to long test times). Can you wait for the onload event instead? https://codereview.chromium.org/2733083004/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:17: assert_false( gotErrorEvent , " Load Event Fired"); This could just be assert(!gotErrorEvent), no?
I'll wait for Bernhard to approve, I'm basically happy with that behavior
Hello Bernhard and jochen, Please find the updated changelist and my comments. Thanks, George. https://codereview.chromium.org/2733083004/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:7: style="display:none" ></object> On 2017/04/09 20:32:24, Bernhard Bauer wrote: > This is formatted pretty badly. Please familiarize yourself with the Web style > guide at > https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md > and format your code accordingly. Done. https://codereview.chromium.org/2733083004/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:13: document.getElementById("objectimage").onerror = imageErrorHandler; On 2017/04/09 20:32:24, Bernhard Bauer wrote: > I don't think you need to chain these two tests together. Maybe make two > separate tests? Done. Second test added. https://codereview.chromium.org/2733083004/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:15: testobject.step_timeout(function() { On 2017/04/09 20:32:24, Bernhard Bauer wrote: > Using timeouts isn't ideal (sometimes tests can legitimately take longer, > leading to flakiness, or the timeout can be too long, leading to long test > times). Can you wait for the onload event instead? Done. Please note that though the load event is being waited for in object-onload-placeholder.html, the load event is fired by the widget implementation(eg: the gif widget does emit a load event, a swf widget does not), so for the purpose of this change maybe a timeout is required to check that an error event is not fired by the new code. I will leave the decision to you. If any changes are required in the test scenario, please advice and I can make the necessary changes. https://codereview.chromium.org/2733083004/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:17: assert_false( gotErrorEvent , " Load Event Fired"); On 2017/04/09 20:32:24, Bernhard Bauer wrote: > This could just be assert(!gotErrorEvent), no? Done. Code Removed.
https://codereview.chromium.org/2733083004/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:9: " for <object> elements"); Broken lines should be indented _four_ spaces. I really should not have to point that out three separate times. https://codereview.chromium.org/2733083004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:11: testobject.done(); Scopes should be indented _two_ spaces. Same goes for the corresponding lines in the other test. https://codereview.chromium.org/2733083004/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:11: testobject.done(); Can you add an error handler that fails the test? And then maybe call testobject.done() via setTimeout(0), just to make sure the message loop gets flushed? If the error handler would fire later than that, we would miss it, but that's just not a thing that can be easily captured in tests.
Hello Brenhard, Thanks for the review comments. I have updated the commit as per your suggestions. I hope all the nits have been sorted. Thanks, George. https://codereview.chromium.org/2733083004/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:9: " for <object> elements"); On 2017/04/13 15:48:42, Bernhard Bauer wrote: > Broken lines should be indented _four_ spaces. I really should not have to point > that out three separate times. Done. Sorry this keeps happening.Somehow, I keep saving it with 4 line indentation. https://codereview.chromium.org/2733083004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onerror-placeholder.html:11: testobject.done(); On 2017/04/13 15:48:42, Bernhard Bauer wrote: > Scopes should be indented _two_ spaces. Same goes for the corresponding lines in > the other test. Done.
https://codereview.chromium.org/2733083004/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:11: testobject.done(); On 2017/04/13 15:48:42, Bernhard Bauer wrote: > Can you add an error handler that fails the test? And then maybe call > testobject.done() via setTimeout(0), just to make sure the message loop gets > flushed? If the error handler would fire later than that, we would miss it, but > that's just not a thing that can be easily captured in tests. Done.
https://codereview.chromium.org/2733083004/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:14: setTimeout(function () { I think the timeout is in the wrong place here. You want the timeout in the load handler to delay passing the test (in case the error event ends up on the message loop right after it). Also, nit: Using arrow syntax (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Functions/Arro...) for the function would look a bit nicer. https://codereview.chromium.org/2733083004/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:16: assert_true(false); I would use assert_unreached(<message>), as that conveys the intent more clearly than asserting that true is false :) and it lets you specify a nice error message. https://codereview.chromium.org/2733083004/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:18: testobject.done(); This call is also unnecessary. https://codereview.chromium.org/2733083004/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:19: },0); Nit: space after comma.
Hello Bernhard, Thanks for your review comments. I have made the changes as you have suggested. Thanks, George. https://codereview.chromium.org/2733083004/diff/220001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:14: setTimeout(function () { On 2017/04/18 10:08:02, Bernhard Bauer wrote: > I think the timeout is in the wrong place here. You want the timeout in the load > handler to delay passing the test (in case the error event ends up on the > message loop right after it). > > Also, nit: Using arrow syntax > (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Functions/Arro...) > for the function would look a bit nicer. Done. https://codereview.chromium.org/2733083004/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:16: assert_true(false); On 2017/04/18 10:08:02, Bernhard Bauer wrote: > I would use assert_unreached(<message>), as that conveys the intent more clearly > than asserting that true is false :) and it lets you specify a nice error > message. Done. https://codereview.chromium.org/2733083004/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:18: testobject.done(); On 2017/04/18 10:08:02, Bernhard Bauer wrote: > This call is also unnecessary. Done. https://codereview.chromium.org/2733083004/diff/220001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:19: },0); On 2017/04/18 10:08:02, Bernhard Bauer wrote: > Nit: space after comma. Done.
https://codereview.chromium.org/2733083004/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:11: setTimeout( () => {}, 0); This just posts an empty callback. You need to move the `testobject.done()` call into the callback. Also, I would remove the space before the empty parameter list. https://codereview.chromium.org/2733083004/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:15: assert_unreached("Error event received "); Nit: No space before the end of the string.
Hello Bernhard, Thanks for the review comments. I have updated the changes as per your feedback. Do let me know if this is ok. Thanks, George. https://codereview.chromium.org/2733083004/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:11: setTimeout( () => {}, 0); On 2017/04/18 14:46:39, Bernhard Bauer wrote: > This just posts an empty callback. You need to move the `testobject.done()` call > into the callback. Also, I would remove the space before the empty parameter > list. Done. https://codereview.chromium.org/2733083004/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:15: assert_unreached("Error event received "); On 2017/04/18 14:46:39, Bernhard Bauer wrote: > Nit: No space before the end of the string. Done.
Almost there :) https://codereview.chromium.org/2733083004/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:11: setTimeout(() => {testobject.done();}, 0); For readability, move the done() call onto the next line? (Indent it by two spaces because it's a nested scope).
Hello Bernhard, Thanks for the comments. Glad to know I am getting there. I have a had a good learning about the google coding style for this bug. Do let me know if the uploaded patch set is ok. Cheers, George. https://codereview.chromium.org/2733083004/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html (right): https://codereview.chromium.org/2733083004/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/plugins/object-onload-placeholder.html:11: setTimeout(() => {testobject.done();}, 0); On 2017/04/18 15:27:02, Bernhard Bauer wrote: > For readability, move the done() call onto the next line? (Indent it by two > spaces because it's a nested scope). Done.
bauerb@chromium.org changed reviewers: - waffles@chromium.org
LGTM! Moving Joshua to CC as previously mentioned.
Hmph, looks like non-l-g-t-m-ing reviewers are sticky :-/
On 2017/04/18 15:37:54, Bernhard Bauer wrote: > LGTM! Moving Joshua to CC as previously mentioned. Looking at Joshua's message it seemed that he wanted to do a final manual test on this patchset. Given the time we've already invested, I'd recommend just waiting until he gets back on Apr 24 Apologies to George for further delay, but I think it's worth being meticulous, given the impact.
On 2017/04/18 16:59:31, tommycli wrote: > On 2017/04/18 15:37:54, Bernhard Bauer wrote: > > LGTM! Moving Joshua to CC as previously mentioned. > > Looking at Joshua's message it seemed that he wanted to do a final manual test > on this patchset. Given the time we've already invested, I'd recommend just > waiting until he gets back on Apr 24 > > Apologies to George for further delay, but I think it's worth being meticulous, > given the impact. Hello Bernhard and Tommycli, Thanks for the LGTM. No issues with me for the wait. I will scout for a different issue meanwhile. Thanks, George.
jochen@chromium.org changed reviewers: + waffles@chromium.org
lgtm
On 2017/04/20 09:43:54, jochen (slow until May 2) wrote: > lgtm Thanks Joshua. Anyone else who should LGTM for the patch to be integrated? Thanks, George.
On 2017/04/25 13:48:13, George Joseph wrote: > On 2017/04/20 09:43:54, jochen (slow until May 2) wrote: > > lgtm > > Thanks Joshua. Anyone else who should LGTM for the patch to be integrated? > > Thanks, > George. waffles@ (Joshua) still needs to L.G.T.M.
On 2017/04/25 14:59:28, laforge wrote: > On 2017/04/25 13:48:13, George Joseph wrote: > > On 2017/04/20 09:43:54, jochen (slow until May 2) wrote: > > > lgtm > > > > Thanks Joshua. Anyone else who should LGTM for the patch to be integrated? > > > > Thanks, > > George. > > waffles@ (Joshua) still needs to L.G.T.M. George, can you resync to head so that I can test? The current patchset has several merge conflicts.
On 2017/04/25 19:39:34, waffles wrote: > On 2017/04/25 14:59:28, laforge wrote: > > On 2017/04/25 13:48:13, George Joseph wrote: > > > On 2017/04/20 09:43:54, jochen (slow until May 2) wrote: > > > > lgtm > > > > > > Thanks Joshua. Anyone else who should LGTM for the patch to be integrated? > > > > > > Thanks, > > > George. > > > > waffles@ (Joshua) still needs to L.G.T.M. > > George, can you resync to head so that I can test? The current patchset has > several merge conflicts. Hello Joshua, Thanks for getting back to me. I have uploaded the patch after rebase. Thanks, George.
On 2017/04/26 23:42:19, George Joseph wrote: > On 2017/04/25 19:39:34, waffles wrote: > > On 2017/04/25 14:59:28, laforge wrote: > > > On 2017/04/25 13:48:13, George Joseph wrote: > > > > On 2017/04/20 09:43:54, jochen (slow until May 2) wrote: > > > > > lgtm > > > > > > > > Thanks Joshua. Anyone else who should LGTM for the patch to be integrated? > > > > > > > > Thanks, > > > > George. > > > > > > waffles@ (Joshua) still needs to L.G.T.M. > > > > George, can you resync to head so that I can test? The current patchset has > > several merge conflicts. > > Hello Joshua, > > Thanks for getting back to me. I have uploaded the patch after rebase. > > Thanks, > George. Hello waffles, Do let me know if there is any extra work to do on the updated patch after a resync(in case there is a merge error again). Thanks, George.
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/02 10:59:19, George Joseph wrote: > On 2017/04/26 23:42:19, George Joseph wrote: > > On 2017/04/25 19:39:34, waffles wrote: > > > On 2017/04/25 14:59:28, laforge wrote: > > > > On 2017/04/25 13:48:13, George Joseph wrote: > > > > > On 2017/04/20 09:43:54, jochen (slow until May 2) wrote: > > > > > > lgtm > > > > > > > > > > Thanks Joshua. Anyone else who should LGTM for the patch to be > integrated? > > > > > > > > > > Thanks, > > > > > George. > > > > > > > > waffles@ (Joshua) still needs to L.G.T.M. > > > > > > George, can you resync to head so that I can test? The current patchset has > > > several merge conflicts. > > > > Hello Joshua, > > > > Thanks for getting back to me. I have uploaded the patch after rebase. > > > > Thanks, > > George. > > Hello waffles, Do let me know if there is any extra work to do on the updated > patch after > a resync(in case there is a merge error again). > > Thanks, > George. lgtm - my testing shows no regression in JIT flash loading. Thank you very much for your patience (and for contributing the patch in the first place)! (Believe it or not, I haven't just been sitting on the change, I encountered two bugs unrelated to your change that were interfering with the end-to-end test.) Because that took me so long, there is another minor merge conflict. If you sync and upload a final patch set, we can start landing it. Thanks again!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/05/02 17:21:06, waffles wrote: > On 2017/05/02 10:59:19, George Joseph wrote: > > On 2017/04/26 23:42:19, George Joseph wrote: > > > On 2017/04/25 19:39:34, waffles wrote: > > > > On 2017/04/25 14:59:28, laforge wrote: > > > > > On 2017/04/25 13:48:13, George Joseph wrote: > > > > > > On 2017/04/20 09:43:54, jochen (slow until May 2) wrote: > > > > > > > lgtm > > > > > > > > > > > > Thanks Joshua. Anyone else who should LGTM for the patch to be > > integrated? > > > > > > > > > > > > Thanks, > > > > > > George. > > > > > > > > > > waffles@ (Joshua) still needs to L.G.T.M. > > > > > > > > George, can you resync to head so that I can test? The current patchset > has > > > > several merge conflicts. > > > > > > Hello Joshua, > > > > > > Thanks for getting back to me. I have uploaded the patch after rebase. > > > > > > Thanks, > > > George. > > > > Hello waffles, Do let me know if there is any extra work to do on the updated > > patch after > > a resync(in case there is a merge error again). > > > > Thanks, > > George. > > lgtm - my testing shows no regression in JIT flash loading. > > Thank you very much for your patience (and for contributing the patch in the > first place)! (Believe it or not, I haven't just been sitting on the change, I > encountered two bugs unrelated to your change that were interfering with the > end-to-end test.) > > Because that took me so long, there is another minor merge conflict. If you sync > and upload a final patch set, we can start landing it. Thanks again! Hello Waffles, Thanks for the lgtm. I have uploaded the latest patchset after reconciling the new changes. Thanks, George.
The CQ bit was checked by kottackal.george@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by waffles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, bauerb@chromium.org, jochen@chromium.org, waffles@chromium.org Link to the patchset: https://codereview.chromium.org/2733083004/#ps320001 (title: "Emit error events if the loading of an object element failed")
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": 320001, "attempt_start_ts": 1493840094511990, "parent_rev": "965721b2c4c2701a64740b0187824ab09b11954e", "commit_rev": "f4335e87b019d74e3d803318101922bb40cec682"}
Message was sent while issue was closed.
Description was changed from ========== Emit error events if the loading of an object element failed Dispatch onerror/error events in the case of a placeholder being instantiated instead of an actual plugin. BUG=445557 ========== to ========== Emit error events if the loading of an object element failed Dispatch onerror/error events in the case of a placeholder being instantiated instead of an actual plugin. BUG=445557 Review-Url: https://codereview.chromium.org/2733083004 Cr-Commit-Position: refs/heads/master@{#469081} Committed: https://chromium.googlesource.com/chromium/src/+/f4335e87b019d74e3d8033181019... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/f4335e87b019d74e3d8033181019... |