<webview>: Add name attribute

<webview>: Add name attribute

This change requires this WebKit patch: https://bugs.webkit.org/show_bug.cgi?id=104404 

This change enables access to the guest's window's name attribute from the embedder <webview>. It also enables changes to the window name attribute from the embedder WebContents.

BUG=140316
Test=BrowserPluginHostTest.ChangeWindowName

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

[Fady Samuel, 2012-12-12 22:27:26]

window.open was growing really rapidly in complexity so I've decided to start
splitting it up.

This patch implements the name attribute. There is a WebKit side change that is
required for this change.

[Charlie Reis, 2012-12-13 01:11:41]

Just a few thoughts so far.

It seems strange to me to send this info via RenderView and normal
view_messages.h IPCs, but then only store it in the browser plugin.  Would it be
saner to store this as WebContents::name_?  We'll need to use it there
eventually anyway.

Also, we don't need this in M25, right?  Just trying to prioritize things for
the branch point accordingly.

https://codereview.chromium.org/11554030/diff/2001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/11554030/diff/2001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:128: if
(!this.objectNode_.hasAttribute(mutation.attributeName)) {
I'm not entirely sure which type of attribute change this is preventing.  (Is it
coming from within the BrowserPlugin or from JS code?)  Perhaps a comment with
an example of why we need this would help?

https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:84: name));
nit: This last arg can go on previous line, which would be easier to read. 
(Only function signatures need each arg on a separate line.)

https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:574:
UpdateFrameName(frame_id, is_main_frame, "");
Though I wish the web platform worked this way, it's not true.  A window's name
sticks around after navigations.  The name applies to the "browsing context,"
not the document.

https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:655:
web_contents()->GetRenderViewHost()->Send(new ViewMsg_SetName(
We should cache the RVH in a local variable, since this virtual method is called
at least 3 times here.

[Fady Samuel, 2012-12-13 01:54:35]

This is not needed for M25. I'll address your comments in the morning. We
can hold off landing this until next week.

Fady

On Wed, Dec 12, 2012 at 8:11 PM, <creis@chromium.org> wrote:

> Just a few thoughts so far.
>
> It seems strange to me to send this info via RenderView and normal
> view_messages.h IPCs, but then only store it in the browser plugin.  Would
> it be
> saner to store this as WebContents::name_?  We'll need to use it there
> eventually anyway.
>
> Also, we don't need this in M25, right?  Just trying to prioritize things
> for
> the branch point accordingly.
>
>
> https://codereview.chromium.**org/11554030/diff/2001/chrome/**
>
renderer/resources/extensions/**web_view.js<https://codereview.chromium.org/11554030/diff/2001/chrome/renderer/resources/extensions/web_view.js>
> File chrome/renderer/resources/**extensions/web_view.js (right):
>
> https://codereview.chromium.**org/11554030/diff/2001/chrome/**
>
renderer/resources/extensions/**web_view.js#newcode128<https://codereview.chromium.org/11554030/diff/2001/chrome/renderer/resources/extensions/web_view.js#newcode128>
> chrome/renderer/resources/**extensions/web_view.js:128: if
> (!this.objectNode_.**hasAttribute(mutation.**attributeName)) {
> I'm not entirely sure which type of attribute change this is preventing.
>  (Is it coming from within the BrowserPlugin or from JS code?)  Perhaps
> a comment with an example of why we need this would help?
>
> https://codereview.chromium.**org/11554030/diff/2001/**
>
content/browser/browser_**plugin/browser_plugin_guest.cc<https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_plugin/browser_plugin_guest.cc>
> File content/browser/browser_**plugin/browser_plugin_guest.cc (right):
>
> https://codereview.chromium.**org/11554030/diff/2001/**
>
content/browser/browser_**plugin/browser_plugin_guest.**cc#newcode84<https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_plugin/browser_plugin_guest.cc#newcode84>
> content/browser/browser_**plugin/browser_plugin_guest.**cc:84: name));
> nit: This last arg can go on previous line, which would be easier to
> read.  (Only function signatures need each arg on a separate line.)
>
> https://codereview.chromium.**org/11554030/diff/2001/**
>
content/browser/browser_**plugin/browser_plugin_guest.**cc#newcode574<https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_plugin/browser_plugin_guest.cc#newcode574>
> content/browser/browser_**plugin/browser_plugin_guest.**cc:574:
> UpdateFrameName(frame_id, is_main_frame, "");
> Though I wish the web platform worked this way, it's not true.  A
> window's name sticks around after navigations.  The name applies to the
> "browsing context," not the document.
>
> https://codereview.chromium.**org/11554030/diff/2001/**
>
content/browser/browser_**plugin/browser_plugin_guest.**cc#newcode655<https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_plugin/browser_plugin_guest.cc#newcode655>
> content/browser/browser_**plugin/browser_plugin_guest.**cc:655:
> web_contents()->**GetRenderViewHost()->Send(new ViewMsg_SetName(
> We should cache the RVH in a local variable, since this virtual method
> is called at least 3 times here.
>
>
https://codereview.chromium.**org/11554030/<https://codereview.chromium.org/1...
>

[Fady Samuel, 2012-12-13 17:46:45]

Hi Charlie,

I've addressed your comments. I now plumb the name updates from
RenderViewHostImpl to WebContentsImpl and then inform all WebContentsObservers.
BrowserPluginGuest is a WebContentsObserver that overrides the
DidUpdateFrameName method and so it sees those updates.

I still store "name_" in BrowserPluginGuest (I'm investigating the cleanest way
to move this over to WebContentsImpl now). My current thought is to plumb it
through WebContents::CreateParams, and add WebContents::GetName() and
WebContents::SetName(..) methods.

Thoughts?

Thanks,

Fady

https://codereview.chromium.org/11554030/diff/2001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/11554030/diff/2001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:128: if
(!this.objectNode_.hasAttribute(mutation.attributeName)) {
On 2012/12/13 01:11:42, creis wrote:
> I'm not entirely sure which type of attribute change this is preventing.  (Is
it
> coming from within the BrowserPlugin or from JS code?)  Perhaps a comment with
> an example of why we need this would help?

Done.

https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:84: name));
On 2012/12/13 01:11:42, creis wrote:
> nit: This last arg can go on previous line, which would be easier to read. 
> (Only function signatures need each arg on a separate line.)

Done.

https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:574:
UpdateFrameName(frame_id, is_main_frame, "");
On 2012/12/13 01:11:42, creis wrote:
> Though I wish the web platform worked this way, it's not true.  A window's
name
> sticks around after navigations.  The name applies to the "browsing context,"
> not the document.

Removed, thanks.

https://codereview.chromium.org/11554030/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:655:
web_contents()->GetRenderViewHost()->Send(new ViewMsg_SetName(
On 2012/12/13 01:11:42, creis wrote:
> We should cache the RVH in a local variable, since this virtual method is
called
> at least 3 times here.

Done.

[Fady Samuel, 2012-12-13 19:39:45]

I've moved name_ to WebContentsImpl entirely. I've added GetName()/SetName() to
the WebContents public interface. Landing this is not a priority until the M25
cut. Please review at your convenience.

[Charlie Reis, 2012-12-15 00:13:04]

Actually, before I send the next round of comments, I want to take a step back. 
Why do we need the name in the browser process?  (I have my own reasons for
wanting it in WebContents, but I don't think they matter here.)

It seems to me that we just need the webview's name attribute to make it into
the guest renderer process.  Then guest pages can find existing targets without
any other changes (using existing logic in WebKit), and we only need special
BrowserPlugin logic for window.open or targeted navigations if we have to create
a new <webview> for it.

What am I missing?  :)

[Fady Samuel, 2013-01-04 19:07:38]

PTAL: Changed according to our offline discussion prior to the holidays. The
name attribute messages are no longer plumbed through WebContents.

Made some changes to accommodate the refactorings I made right before the
holidays.

[Charlie Reis, 2013-01-04 21:36:33]

I think this looks pretty good.  Just one question about how we store the name.

https://codereview.chromium.org/11554030/diff/21001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/11554030/diff/21001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.h:225: // Sets the name of
the guest so that other guests of the same partition can
nit: guests in the same partition

https://codereview.chromium.org/11554030/diff/21001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11554030/diff/21001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2728: UTF16ToUTF8(name)));
Just checking: are we ok with this conversion?  I'm not sure what the rules are
on name strings.  It could potentially prevent windows with non-UTF8 names from
being found.

[Fady Samuel, 2013-01-08 16:19:07]

PTAL.

https://codereview.chromium.org/11554030/diff/21001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/11554030/diff/21001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.h:225: // Sets the name of
the guest so that other guests of the same partition can
On 2013/01/04 21:36:33, creis wrote:
> nit: guests in the same partition

Done.

https://codereview.chromium.org/11554030/diff/21001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11554030/diff/21001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2728: UTF16ToUTF8(name)));
On 2013/01/04 21:36:33, creis wrote:
> Just checking: are we ok with this conversion?  I'm not sure what the rules
are
> on name strings.  It could potentially prevent windows with non-UTF8 names
from
> being found.

Judging by this: http://www.w3.org/TR/html4/present/frames.html#adef-name-FRAME

and this: http://www.w3.org/TR/html4/types.html#type-cdata

It seems this is our answer:

"ID and NAME tokens must begin with a letter ([A-Za-z]) and may be followed by
any number of letters, digits ([0-9]), hyphens ("-"), underscores ("_"), colons
(":"), and periods (".")."

It sounds like we don't need to keep this name in a string16 to do what a
frame's name can do.

[Charlie Reis, 2013-01-08 21:18:45]

LGTM

https://codereview.chromium.org/11554030/diff/21001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11554030/diff/21001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2728: UTF16ToUTF8(name)));
On 2013/01/08 16:19:07, Fady Samuel wrote:
> On 2013/01/04 21:36:33, creis wrote:
> > Just checking: are we ok with this conversion?  I'm not sure what the rules
> are
> > on name strings.  It could potentially prevent windows with non-UTF8 names
> from
> > being found.
> 
> Judging by this:
http://www.w3.org/TR/html4/present/frames.html#adef-name-FRAME
> 
> and this: http://www.w3.org/TR/html4/types.html#type-cdata
> 
> It seems this is our answer:
> 
> "ID and NAME tokens must begin with a letter ([A-Za-z]) and may be followed by
> any number of letters, digits ([0-9]), hyphens ("-"), underscores ("_"),
colons
> (":"), and periods (".")."
> 
> It sounds like we don't need to keep this name in a string16 to do what a
> frame's name can do.

Great!  Thanks for verifying.

[commit-bot: I haz the power, 2013-01-08 21:21:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11554030/30001

[commit-bot: I haz the power, 2013-01-08 21:21:13]

Presubmit check for 11554030-30001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/common/browser_plugin_messages.h
    content/common/view_messages.h

Presubmit checks took 1.8s to calculate.

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

[Fady Samuel, 2013-01-08 22:02:46]

+cdn for IPC security review.

[Cris Neckar, 2013-01-08 22:32:28]

IPC security audit LGTM

[commit-bot: I haz the power, 2013-01-08 22:35:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11554030/30001

[commit-bot: I haz the power, 2013-01-09 00:10:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
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-01-09 00:10:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11554030/34003

[commit-bot: I haz the power, 2013-01-09 00:54:14]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, chromedriver2_unittests,
content_browsertests, content_unittests, crypto_unittests, gpu_unittests,
interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests,
nacl_integration, net_unittests, ppapi_unittests, printing_unittests,
remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests,
unit_tests