Adding parsing for partition and persistence attributes to the <browser> tag

Adding parsing for partition and persistence attributes to the <browser> tag

BUG=145500

[nasko, 2012-08-31 23:09:35]

Rob,
Can you review this change? I've added support for "persist" and "partition"
attributes on the <browser> tag. These will be used for isolation and follow-up
patches will actually start using the values.
Thanks,
Nasko

[rjkroege, 2012-08-31 23:41:25]

lgtm

[rjkroege, 2012-08-31 23:41:27]

lgtm

lgtm

[nasko, 2012-08-31 23:43:13]

Including jamesr@ for OWNERS review of content/renderer.

[nasko, 2012-09-10 16:20:20]

James, can you review this CL?

[jamesr, 2012-09-10 17:45:56]

I'm not familiar enough with storage to review this.  Perhaps michaeln@ or
ericu@ could take a look first?

[nasko, 2012-09-10 17:49:48]

Updated the title to better reflect the code. I've only added support for the
attributes on the tag, haven't wired them to do anything yet.

[michaeln, 2012-09-10 20:50:02]

I don't understand the purpose of this change or how it fits in with creis's and
ajwong's storage partitioning work. Looks like the intent is to add support for
additional attributes on the <object> tags used to instance embedded
browserplugins...

<object id='browserplugin' width='640px' height='480px'
  src='foo' type='%s' partition='someid' persist>

I don't understand why these values would be needed in HTML content as an
embedded browser always gets bound to the partition of the embedder.

[nasko, 2012-09-10 21:10:24]

On 2012/09/10 20:50:02, michaeln wrote:
> I don't understand the purpose of this change or how it fits in with creis's
and
> ajwong's storage partitioning work. Looks like the intent is to add support
for
> additional attributes on the <object> tags used to instance embedded
> browserplugins...
> 
> <object id='browserplugin' width='640px' height='480px'
>   src='foo' type='%s' partition='someid' persist>
> 
> I don't understand why these values would be needed in HTML content as an
> embedded browser always gets bound to the partition of the embedder.

Please refer to this doc for design details:
https://docs.google.com/a/google.com/document/d/1w8uZLdf6gyhJeLaGz4AD1gad6udh....

This is the first step in allowing developers to specify storage partitions and
persistence levels. The next step is to integrate those values with the work
ajwong@ is doing.

[michaeln, 2012-09-10 23:01:42]

thnx for the pointer to that doc, i'll share it with the chrome-owp-storage list
too

So the idea is that an 'app' can establish any number of partitions which are
optionally persistent or nonpersistent, and embed <browsers> tied to those
partitions as they see fit.

I see the current 'isolated apps' scheme (the thing being implemented by ajwong)
as somewhat different than this. In that scheme, an 'app' declares itself as
'isolated' and any embedded <browsers> (and xorigin fetches performed by the app
proper) are tied to storage resources independent of the regular web.

What timeframe do you have in mind for the ideas outlined in that doc?

https://chromiumcodereview.appspot.com/10912054/diff/2001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10912054/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:113: if (has_navigated_ ||
!partition_id_.empty())
is there a reason to prevent setting to a different value prior to having
navigated?

https://chromiumcodereview.appspot.com/10912054/diff/2001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/10912054/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:26: enum BoolAttribute {
Doesn't look like this needs to be public. Actually is the fancy tri-state type
really needed? So long as the value isn't mutated after the plugin has
navigated, what's the harm in changing it.

[nasko, 2012-09-10 23:16:08]

Actually, the scheme ajwong is developing and this are in the same direction,
using the same underlying concepts. In apps v2 world, apps do not declare
themselves isolated, this is done on their behalf and they have no choice in the
matter. You are correct that the app will be independent from the regular web.

Now, under a specific app, each browser tag can get a partition and if the app
declares more than one browser tag with the same partition, those will share
their state. This state will be specific to the app, independent of the regular
web, as well as other apps.

The goal is to have this browser tag storage isolation be done as soon as the
rest of the pieces are in (new browser tag code and the storage contexts for all
types of persisted data). I'm trying to see if I can make M23, but if not, M24
is definitely the target.

https://chromiumcodereview.appspot.com/10912054/diff/2001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10912054/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:113: if (has_navigated_ ||
!partition_id_.empty())
On 2012/09/10 23:01:43, michaeln wrote:
> is there a reason to prevent setting to a different value prior to having
> navigated?

We want to alert the developers that this is a one time set property. I don't
see a reason why this should be set multiple times prior to navigation.

https://chromiumcodereview.appspot.com/10912054/diff/2001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/10912054/diff/2001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.h:26: enum BoolAttribute {
On 2012/09/10 23:01:43, michaeln wrote:
> Doesn't look like this needs to be public. Actually is the fancy tri-state
type
> really needed? So long as the value isn't mutated after the plugin has
> navigated, what's the harm in changing it.

We want to raise an exception if JavaScript is used to modify the value, in
order to make it visible to developers that this is unsupported. Hence, I wanted
to know the case of "unset".

[nasko, 2012-09-10 23:16:54]

Adding ajwong@, since his work is referenced.

[awong, 2012-09-10 23:22:47]

On 2012/09/10 23:16:54, nasko wrote:
> Adding ajwong@, since his work is referenced.

Drive-by comments:
 1) Where are we doing the validation of the partition id? IIRC, we weren't just
allowing random freeform strings
 2) Is there a distinction between partition_id="" and partition_id unset?

[michaeln, 2012-09-10 23:51:03]

These changes really don't directly affect corners of code i typically poke at,
so i'll remove myself from the reviewers list... but i can see derived
requirements coming down the pipe that do affect corners of code i work on more
directly so i'm real glad to have been cc'd on this one.

On 2012/09/10 23:16:08, nasko wrote:
> Actually, the scheme ajwong is developing and this are in the same direction,
> using the same underlying concepts. In apps v2 world, apps do not declare
> themselves isolated, this is done on their behalf and they have no choice in
the
> matter. You are correct that the app will be independent from the regular web.

Is there any case where an app can embed a <browser> that is not independent of
the regular web? Just curious.

> I'm trying to see if I can make M23...

Yikes... feels like a stretch. For nonpersistent partitions we may be able to
reuse the same underpinnings as incognito.

> We want to raise an exception if JavaScript is used to modify the value...

I see, so if the user invokes the setters via script after having provided
values via attributes you want to complain about it.