Fix the bug webviews can not be loaded

Fix the bug webviews can not be loaded

Webviews are not loaded after inserting a node which contains <webview> tags
Contributed by yupingx.chen@intel.com


BUG=222663
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193320

[Yang Gu, 2013-03-21 07:26:16]

Hi all,
Thank you in advance

[Finnur, 2013-03-21 10:23:14]

I'm not familiar with this code. Looks like kalman (already CC-ed) was the last
person to change it, so he's probably a better candidate to review.

[Hongbo Min, 2013-03-21 12:38:24]

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/tag_watcher.js (right):

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/tag_watcher.js:28: queryTags(addedNode,
tagName);
It is problematic here since the callback 'cb' will be invoked twice if the
addedNode is a <webview> tag.

Try to use the following code in the loop:

  for (var i = 0, addedNode; addedNode = mutation.addedNodes[i]; i++) {
    queryTags(addedNode, tagName);
  }

[Hongbo Min, 2013-03-21 12:46:15]

Please jeremya for reviewing.

[not at google - send to devlin, 2013-03-21 15:34:27]

I only touched the code to remove references to array.forEach. Removing myself
as a reviewer, this is fsamuel's area.

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/tag_watcher.js (right):

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/tag_watcher.js:28: queryTags(addedNode,
tagName);
On 2013/03/21 12:38:24, Hongbo Min wrote:
> It is problematic here since the callback 'cb' will be invoked twice if the
> addedNode is a <webview> tag.
> 
> Try to use the following code in the loop:
> 
>   for (var i = 0, addedNode; addedNode = mutation.addedNodes[i]; i++) {
>     queryTags(addedNode, tagName);
>   }
> 

use forEach (as in line 23) perhaps

[Yang Gu, 2013-03-22 01:58:32]

On 2013/03/21 12:38:24, Hongbo Min wrote:
>
https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
> File chrome/renderer/resources/extensions/tag_watcher.js (right):
> 
>
https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
> chrome/renderer/resources/extensions/tag_watcher.js:28: queryTags(addedNode,
> tagName);
> It is problematic here since the callback 'cb' will be invoked twice if the
> addedNode is a <webview> tag.
> 
> Try to use the following code in the loop:
> 
>   for (var i = 0, addedNode; addedNode = mutation.addedNodes[i]; i++) {
>     queryTags(addedNode, tagName);
>   }

The function querySelectorAll will not contain the node itself, I have tested
it.

[Yang Gu, 2013-03-22 02:01:34]

Hi Jeremya,

Could you please take a second to review the patch?
Thank you.

[jeremya, 2013-03-22 02:08:35]

On 2013/03/22 02:01:34, Yang Gu wrote:
> Hi Jeremya,
> 
> Could you please take a second to review the patch?
> Thank you.

The idea looks good, but please look at using forEach as kalman suggested.

[Fady Samuel, 2013-03-23 19:53:59]

I don't understand the problem you're trying to fix. Could you please provide a
test?

[Yang Gu, 2013-03-25 01:20:33]

On 2013/03/23 19:53:59, Fady Samuel wrote:
> I don't understand the problem you're trying to fix. Could you please provide
a
> test?

The attachment in the issue http://crbug.com/222663 is the test case.

[Fady Samuel, 2013-03-25 14:14:21]

Cool, once Kalman's comment is addressed LGTM.

[not at google - send to devlin, 2013-03-25 23:35:34]

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/tag_watcher.js (right):

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/tag_watcher.js:12: function
queryTags(queryNode, targetTagName) {
passing targetTagName in here is unnecessary since it's the same as tagName.

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/tag_watcher.js:28: queryTags(addedNode,
tagName);
On 2013/03/21 12:38:24, Hongbo Min wrote:
> It is problematic here since the callback 'cb' will be invoked twice if the
> addedNode is a <webview> tag.
> 
> Try to use the following code in the loop:
> 
>   for (var i = 0, addedNode; addedNode = mutation.addedNodes[i]; i++) {
>     queryTags(addedNode, tagName);
>   }
> 

I did a quick check and e.querySelectorAll doesn't include the element it was
called on, so addedNode does need to be checked explicitly.

My forEach comment was a nit:

forEach(queryNode.querySelectorAll(tagName), function(i, node) {
  cb(node);
});

up at queryTags.

[Yang Gu, 2013-03-27 02:21:12]

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/tag_watcher.js (right):

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/tag_watcher.js:28: queryTags(addedNode,
tagName);
On 2013/03/21 15:34:27, kalman wrote:
> On 2013/03/21 12:38:24, Hongbo Min wrote:
> > It is problematic here since the callback 'cb' will be invoked twice if the
> > addedNode is a <webview> tag.
> > 
> > Try to use the following code in the loop:
> > 
> >   for (var i = 0, addedNode; addedNode = mutation.addedNodes[i]; i++) {
> >     queryTags(addedNode, tagName);
> >   }
> > 
> 
> use forEach (as in line 23) perhaps

Done.

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/tag_watcher.js:28: queryTags(addedNode,
tagName);
On 2013/03/25 23:35:34, kalman wrote:
> On 2013/03/21 12:38:24, Hongbo Min wrote:
> > It is problematic here since the callback 'cb' will be invoked twice if the
> > addedNode is a <webview> tag.
> > 
> > Try to use the following code in the loop:
> > 
> >   for (var i = 0, addedNode; addedNode = mutation.addedNodes[i]; i++) {
> >     queryTags(addedNode, tagName);
> >   }
> > 
> 
> I did a quick check and e.querySelectorAll doesn't include the element it was
> called on, so addedNode does need to be checked explicitly.
> 
> My forEach comment was a nit:
> 
> forEach(queryNode.querySelectorAll(tagName), function(i, node) {
>   cb(node);
> });
> 
> up at queryTags.

There is a bug in the forEach function,  querySelectorAll will return NodeList
object, which should be handled by the same way as arrays. I have submitted
another patch for this issue(https://codereview.chromium.org/13034005)

[Yang Gu, 2013-03-27 02:24:10]

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/tag_watcher.js (right):

https://codereview.chromium.org/12465019/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/tag_watcher.js:12: function
queryTags(queryNode, targetTagName) {
On 2013/03/25 23:35:34, kalman wrote:
> passing targetTagName in here is unnecessary since it's the same as tagName.

Done.

[commit-bot: I haz the power, 2013-03-28 06:48:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yang.gu@intel.com/12465019/15001

[commit-bot: I haz the power, 2013-03-28 06:48:48]

Presubmit check for 12465019-15001 failed and returned exit status 1.

INFO:root:Found 1 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/renderer/resources/extensions/tag_watcher.js

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Yang Gu, 2013-03-28 07:31:59]

Hi Jeremya, Kalman
The patch is updated. Pls have a review, thanks.

[not at google - send to devlin, 2013-04-02 01:08:31]

lgtm

[Yang Gu, 2013-04-02 01:50:57]

Hi Jeremya, Hongbo
Could you please take a second to review the patch?
Thank you.

[jeremya, 2013-04-04 20:34:33]

lgtm with nits

https://codereview.chromium.org/12465019/diff/15001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/tag_watcher.js (right):

https://codereview.chromium.org/12465019/diff/15001/chrome/renderer/resources...
chrome/renderer/resources/extensions/tag_watcher.js:12: function
queryTags(queryNode) {
I think this would be better named findChildTags.

[Yang Gu, 2013-04-07 06:06:08]

https://codereview.chromium.org/12465019/diff/15001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/tag_watcher.js (right):

https://codereview.chromium.org/12465019/diff/15001/chrome/renderer/resources...
chrome/renderer/resources/extensions/tag_watcher.js:12: function
queryTags(queryNode) {
On 2013/04/04 20:34:33, jeremya wrote:
> I think this would be better named findChildTags.

Done.

[commit-bot: I haz the power, 2013-04-07 06:06:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yang.gu@intel.com/12465019/25001

[commit-bot: I haz the power, 2013-04-07 06:14:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-10 02:31:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yang.gu@intel.com/12465019/35001

[commit-bot: I haz the power, 2013-04-10 04:30:41]

Change committed as 193320

[mtomasz, 2013-04-12 06:46:29]

On 2013/04/10 04:30:41, I haz the power (commit-bot) wrote:
> Change committed as 193320

I guess this change causes lots of javascript errors. Please run a wallpaper
picker and see the javascript console. This problem is much more severe in
Files.app.

[mtomasz, 2013-04-12 07:35:59]

On 2013/04/12 06:46:29, mtomasz wrote:
> On 2013/04/10 04:30:41, I haz the power (commit-bot) wrote:
> > Change committed as 193320
> 
> I guess this change causes lots of javascript errors. Please run a wallpaper
> picker and see the javascript console. This problem is much more severe in
> Files.app.

I've prepared fix for this:
https://codereview.chromium.org/14222005/

[Yang Gu, 2013-04-12 07:56:49]

On 2013/04/12 06:46:29, mtomasz wrote:
> On 2013/04/10 04:30:41, I haz the power (commit-bot) wrote:
> > Change committed as 193320
> 
> I guess this change causes lots of javascript errors. Please run a wallpaper
> picker and see the javascript console. This problem is much more severe in
> Files.app.

Thank Tomasz for the patch(https://codereview.chromium.org/14222005/) to fix the
problem.