Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10928237:
Add support for parsing a 'partition' attribute on the <browser> tag. (Closed)
|
Created:
8 years, 3 months ago by nasko Modified:
8 years, 3 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd support for parsing a 'partition' attribute on the <browser> tag.
BUG=145500
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158329
Patch Set 1 #Patch Set 2 : Moving to UTF-8 encoded strings and added test for multibyte characters. #
Total comments: 22
Patch Set 3 : Fixes based on review from Charlie. #
Total comments: 6
Patch Set 4 : Fixing nits and removed restriction on setting attribute more than once. #Patch Set 5 : Removing the attribute encoding test, as it fails to compile on Windows. #Patch Set 6 : Resolving conflicts and removing redudnat variable. #
Total comments: 2
Patch Set 7 : Fixing ordering of variables. #Patch Set 8 : Rebasing on ToT #Patch Set 9 : Another ToT rebase. #Patch Set 10 : Testing #
Messages
Total messages: 23 (0 generated)
Initial patch for <browser> tag to support parsing of a 'partition' attribute to support storage isolation.
On 2012/09/17 19:01:03, nasko wrote: > Initial patch for <browser> tag to support parsing of a 'partition' attribute to > support storage isolation. No specific comments really, I'll leave it up to Charlie. However, this is very cool! Thanks Nasko, I'm glad to see the start of this work! Please let me know if you have any questions!
Looks like this CL supercedes http://codereview.chromium.org/10912054/. Can the older one be closed out now?
On 2012/09/17 19:45:01, michaeln wrote: > Looks like this CL supercedes http://codereview.chromium.org/10912054/. Can the > older one be closed out now? Yes, you are correct. I've closed the other CL, now that I have this one.
Moved to UTF-8 for storing the string identifier and added specific test for non-ASCII partition attributes.
Nice! https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:111: value.append("persist:"); This should be a constant at the top of the file. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:120: error_message = "The object has already navigated."; Can we provide a little more to help the developer? e.g., (", so its partition ID cannot be changed.") https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:124: error_message = "The partition attribute has already been set."; Why is this an error? Couldn't you safely set it to multiple values before the first navigation? https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:134: // We can use the rest of the string as UTF-8 encoded one. Awesome comment. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:141: error_message = "Invalid partition attribute."; Maybe "Invalid empty partition attribute"? This is the "persist:" case, right? https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:168: // true, which prevents changing the 'partition' attribute. Ah, good point. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin.h (right): https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.h:35: // The partition idetifier string is stored as UTF-8. nit: identifier https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.h:38: std::string& error_message); Please add a comment about |error_message|. Also, this would be a good place to mention that this can only be called before the first navigation takes place. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.h:144: bool has_navigated_; FYI: This will end up being redundant with navigate_src_sent_ from the upcoming larger browser plugin CL (http://codereview.chromium.org/10868012/). We should probably remove this once that lands. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right): https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin_browsertest.cc:37: " src='foo' type='%s' partition='persist:someid'>"; Is it also worth testing one with a partition and no src? That would let us change the partition value before assigning to src, for example.
Fixes based on Charlie's review. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:111: value.append("persist:"); On 2012/09/18 22:09:33, creis wrote: > This should be a constant at the top of the file. Done. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:120: error_message = "The object has already navigated."; On 2012/09/18 22:09:33, creis wrote: > Can we provide a little more to help the developer? e.g., (", so its partition > ID cannot be changed.") Done. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:124: error_message = "The partition attribute has already been set."; On 2012/09/18 22:09:33, creis wrote: > Why is this an error? Couldn't you safely set it to multiple values before the > first navigation? I would like to disallow setting it multiple times. It will be consistent with navigation and the rule can be simple - you can only set the partition attribute once. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:134: // We can use the rest of the string as UTF-8 encoded one. On 2012/09/18 22:09:33, creis wrote: > Awesome comment. Credit goes to the master of awesomeness - ajwong! https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:141: error_message = "Invalid partition attribute."; On 2012/09/18 22:09:33, creis wrote: > Maybe "Invalid empty partition attribute"? This is the "persist:" case, right? Done. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:168: // true, which prevents changing the 'partition' attribute. On 2012/09/18 22:09:33, creis wrote: > Ah, good point. Done. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin.h (right): https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.h:35: // The partition idetifier string is stored as UTF-8. On 2012/09/18 22:09:33, creis wrote: > nit: identifier Done. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.h:38: std::string& error_message); On 2012/09/18 22:09:33, creis wrote: > Please add a comment about |error_message|. Also, this would be a good place to > mention that this can only be called before the first navigation takes place. Done. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.h:144: bool has_navigated_; On 2012/09/18 22:09:33, creis wrote: > FYI: This will end up being redundant with navigate_src_sent_ from the upcoming > larger browser plugin CL (http://codereview.chromium.org/10868012/). We should > probably remove this once that lands. Done. https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right): https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin_browsertest.cc:37: " src='foo' type='%s' partition='persist:someid'>"; On 2012/09/18 22:09:33, creis wrote: > Is it also worth testing one with a partition and no src? That would let us > change the partition value before assigning to src, for example. If you agree with my comment about setting partition only once, regardless of navigation, then this need not be tested I think.
https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:124: error_message = "The partition attribute has already been set."; On 2012/09/18 22:56:20, nasko wrote: > On 2012/09/18 22:09:33, creis wrote: > > Why is this an error? Couldn't you safely set it to multiple values before > the > > first navigation? > > I would like to disallow setting it multiple times. It will be consistent with > navigation and the rule can be simple - you can only set the partition attribute > once. That seems like an unnecessary restriction to me, and I don't agree with that way of stating the rule. ("You can only set the partition attribute once" would allow setting it once after the first navigation.) To me, the rule is "you can only set the partition attribute before the first navigation," which implies you can set it as many times as you want until then. That said, I can't think of a case it would matter. Also, I suppose it's easier to loosen the restriction later (if people object) than to add it. So I'm ok with leaving it if you want the rule to be "you can only set the partition once, and only before the first navigation." https://codereview.chromium.org/10928237/diff/12001/content/renderer/browser_... File content/renderer/browser_plugin/browser_plugin.h (right): https://codereview.chromium.org/10928237/diff/12001/content/renderer/browser_... content/renderer/browser_plugin/browser_plugin.h:37: // This method can successfully called only before the first navigation for nit: can be https://codereview.chromium.org/10928237/diff/12001/content/renderer/browser_... content/renderer/browser_plugin/browser_plugin.h:38: // the instance of BrowserPlugin. If an error occurs, the |error_message| is nit: this instance Also, we should mention that it can only be set once, if we're sticking with that restriction. https://codereview.chromium.org/10928237/diff/12001/content/renderer/browser_... content/renderer/browser_plugin/browser_plugin.h:152: bool persist_storage_; Is it worth exposing this separately, so that callers of GetPartitionAttribute don't have to parse it out again? Or are you planning to do that later?
https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/10928237/diff/5001/content/renderer/browser_p... content/renderer/browser_plugin/browser_plugin.cc:124: error_message = "The partition attribute has already been set."; On 2012/09/19 00:06:31, creis wrote: > On 2012/09/18 22:56:20, nasko wrote: > > On 2012/09/18 22:09:33, creis wrote: > > > Why is this an error? Couldn't you safely set it to multiple values before > > the > > > first navigation? > > > > I would like to disallow setting it multiple times. It will be consistent with > > navigation and the rule can be simple - you can only set the partition > attribute > > once. > > That seems like an unnecessary restriction to me, and I don't agree with that > way of stating the rule. ("You can only set the partition attribute once" would > allow setting it once after the first navigation.) > > To me, the rule is "you can only set the partition attribute before the first > navigation," which implies you can set it as many times as you want until then. > > That said, I can't think of a case it would matter. Also, I suppose it's easier > to loosen the restriction later (if people object) than to add it. So I'm ok > with leaving it if you want the rule to be "you can only set the partition once, > and only before the first navigation." Done. https://codereview.chromium.org/10928237/diff/12001/content/renderer/browser_... File content/renderer/browser_plugin/browser_plugin.h (right): https://codereview.chromium.org/10928237/diff/12001/content/renderer/browser_... content/renderer/browser_plugin/browser_plugin.h:37: // This method can successfully called only before the first navigation for On 2012/09/19 00:06:31, creis wrote: > nit: can be Done. https://codereview.chromium.org/10928237/diff/12001/content/renderer/browser_... content/renderer/browser_plugin/browser_plugin.h:38: // the instance of BrowserPlugin. If an error occurs, the |error_message| is On 2012/09/19 00:06:31, creis wrote: > nit: this instance > > Also, we should mention that it can only be set once, if we're sticking with > that restriction. Done. https://codereview.chromium.org/10928237/diff/12001/content/renderer/browser_... content/renderer/browser_plugin/browser_plugin.h:152: bool persist_storage_; On 2012/09/19 00:06:31, creis wrote: > Is it worth exposing this separately, so that callers of GetPartitionAttribute > don't have to parse it out again? Or are you planning to do that later? I'm not sure if we will have internal callers in RV that will use this information. If we do, I will expose it, but for the time being I'm leaving it as is. This will be sent separately up to the browser process to determine storage partition, but it will likely not go through GetPartitionAttribute.
I've removed the test for the encoding of the partition attribute. It fails to compile on Windows and it was already pointed out that the test doesn't buy us much.
LGTM. Please wait for http://codereview.chromium.org/10868012/ to land and resolve the conflict, though.
On 2012/09/19 22:30:39, creis wrote: > LGTM. Please wait for http://codereview.chromium.org/10868012/ to land and > resolve the conflict, though. The patch has landed. Feel free to land this now :-)
Merged with Fady's changes.
LGTM with one minor nit. How do you plan to carry the partition_id to the browser process? Piggyback BrowserPluginHostMsg_NavigateGuest? https://codereview.chromium.org/10928237/diff/30001/content/renderer/browser_... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/10928237/diff/30001/content/renderer/browser_... content/renderer/browser_plugin/browser_plugin.cc:48: const char* kPersistPrefix = "persist:"; alphabetize.
Yes, the idea so far is to piggy back on the navigate message. It will not only be the partition id, but also the persistence bit. It might be sent only on the first navigation to avoid the extra copies afterwards. https://codereview.chromium.org/10928237/diff/30001/content/renderer/browser_... File content/renderer/browser_plugin/browser_plugin.cc (right): https://codereview.chromium.org/10928237/diff/30001/content/renderer/browser_... content/renderer/browser_plugin/browser_plugin.cc:48: const char* kPersistPrefix = "persist:"; On 2012/09/20 20:49:21, Fady Samuel wrote: > alphabetize. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/10928237/27002
Failed to apply patch for content/renderer/browser_plugin/browser_plugin_bindings.cc: While running patch -p1 --forward --force; patching file content/renderer/browser_plugin/browser_plugin_bindings.cc Hunk #1 FAILED at 41. Hunk #2 succeeded at 69 (offset 10 lines). Hunk #3 succeeded at 196 (offset 26 lines). Hunk #4 succeeded at 205 (offset 26 lines). Hunk #5 succeeded at 235 (offset 26 lines). 1 out of 5 hunks FAILED -- saving rejects to file content/renderer/browser_plugin/browser_plugin_bindings.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/10928237/30004
Failed to apply patch for content/renderer/browser_plugin/browser_plugin_bindings.cc: While running patch -p1 --forward --force; patching file content/renderer/browser_plugin/browser_plugin_bindings.cc Hunk #1 FAILED at 39. Hunk #2 succeeded at 69 (offset 10 lines). Hunk #3 succeeded at 196 (offset 26 lines). Hunk #4 succeeded at 205 (offset 26 lines). Hunk #5 succeeded at 235 (offset 26 lines). 1 out of 5 hunks FAILED -- saving rejects to file content/renderer/browser_plugin/browser_plugin_bindings.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/10928237/31006
Retried try job too often for step(s) crypto_unittests, unit_tests, cacheinvalidation_unittests, remoting_unittests, jingle_unittests, nacl_integration, ipc_tests, interactive_ui_tests, browser_tests, net_unittests, content_unittests, gpu_unittests, base_unittests, sync_integration_tests, media_unittests, safe_browsing_tests, content_browsertests, sql_unittests, printing_unittests, sync_unit_tests, check_deps
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/10928237/39004
Change committed as 158329 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
