Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2835183002:
Provide a method to remove inserted style sheet (Closed)
DescriptionProvide a method to remove inserted style sheet
InsertStyleSheet() returns sequence id. And RemoveInsertedStyleSheet()
removes the inserted style sheet with its id.
TEST=webkit_unit_tests --gtest_fiter=StyleEngineTest.AnalyzedInject, WebDocumentTest.InsertAndRemoveStyleSheet
BUG=608854
Review-Url: https://codereview.chromium.org/2835183002
Cr-Commit-Position: refs/heads/master@{#474919}
Committed: https://chromium.googlesource.com/chromium/src/+/0ffd86f3ca15a4728605fa70435297d334ccc35a
Patch Set 1 : WIP #Patch Set 2 : initial for review #
Total comments: 11
Patch Set 3 : review feedback #
Total comments: 4
Patch Set 4 : Use WebStyleSheetId #Patch Set 5 : rebase #
Messages
Total messages: 44 (26 generated)
Patchset #5 (id:80001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Description was changed from ========== FIX BUG= ========== to ========== [DO NOT MERGE/WIP] InjectAuthorSheet returns cssId Working in progress. BUG=608854 ==========
limasdf@gmail.com changed reviewers: + rdevlin.cronin@chromium.org
== NOT FOR REVIEW ==
Hi Devlin,
Can you share your opinion?
I made StyleEngine::InjectAuthorSheet() return cssId. and I was thinking that,
from extension side, we can use this cssId as a callback parameter of
chrome.tabs.insertCSS().
However, on second thought, I found a problem.
When we call tabs.insertCSS() with 'allFrames: true', e.g.
chrome.tabs.insertCSS(1, { allFrames: true}, function(cssid){});, if a tab has
several Documents and each document generate its cssId, we will receive several
cssId.
at this situation, How should can we handle them?
Sorry for my bad English. Please feel free to ask if you don't get.
rdevlin.cronin@chromium.org changed reviewers: + pdr@chromium.org
On 2017/04/26 18:32:26, limasdf wrote:
> == NOT FOR REVIEW ==
>
> Hi Devlin,
>
> Can you share your opinion?
>
> I made StyleEngine::InjectAuthorSheet() return cssId. and I was thinking that,
> from extension side, we can use this cssId as a callback parameter of
> chrome.tabs.insertCSS().
>
> However, on second thought, I found a problem.
> When we call tabs.insertCSS() with 'allFrames: true', e.g.
> chrome.tabs.insertCSS(1, { allFrames: true}, function(cssid){});, if a tab has
> several Documents and each document generate its cssId, we will receive
several
> cssId.
> at this situation, How should can we handle them?
>
> Sorry for my bad English. Please feel free to ask if you don't get.
There's a few different options here. What we'll probably want to do is have a
unique id exposed to the extension, and internally map that to specific render
frame. We could do that reasonably in two ways:
- Have blink return a non-unique identifier and store a map<unique_id,
std::pair<RenderFrameHost, blink_id>> in the extensions API. This would mean
blink just has to return some incrementing integer.
- Have blink return a globally-unique identifier and store a
map<unique_blink_id, RenderFrameHost> in the extensions API. This is a bit
simpler, since there's no mapping of unique -> blink ids. We can generate a
unique id through something like base::GenerateGUID().
In either case, we will likely want to store a map in the extensions API. It
wouldn't be *strictly* necessary in the case of a globally unique id, but it
would allow us to more quickly catch any errors where extensions pass an invalid
id.
pdr@ may have a preference for one of these approaches over the other.
@pdr, would you share your opinion on devlin's suggestion?
On 2017/05/05 at 17:09:07, limasdf wrote: > @pdr, would you share your opinion on devlin's suggestion? I think both approaches make sense. I don't know enough about this boundary to know if one of these is much simpler than the other.
Description was changed from ========== [DO NOT MERGE/WIP] InjectAuthorSheet returns cssId Working in progress. BUG=608854 ========== to ========== Provide a method to remove inserted style sheet InsertStyleSheet() returns sequence id. And we can remove the inserted style sheet with this id. TEST= BUG=608854 ==========
Description was changed from ========== Provide a method to remove inserted style sheet InsertStyleSheet() returns sequence id. And we can remove the inserted style sheet with this id. TEST= BUG=608854 ========== to ========== Provide a method to remove inserted style sheet InsertStyleSheet() returns sequence id. And RemoveInsertedStyleSheet() removes the inserted style sheet with its id. TEST= BUG=608854 ==========
Patchset #2 (id:220001) has been deleted
Description was changed from ========== Provide a method to remove inserted style sheet InsertStyleSheet() returns sequence id. And RemoveInsertedStyleSheet() removes the inserted style sheet with its id. TEST= BUG=608854 ========== to ========== Provide a method to remove inserted style sheet InsertStyleSheet() returns sequence id. And RemoveInsertedStyleSheet() removes the inserted style sheet with its id. TEST=webkit_unit_tests --gtest_fiter=StyleEngineTest.AnalyzedInject BUG=608854 ==========
Hi Devlin, Pdr. Devlin@, I took the option #1, Have blink return a non-unique identifier(sequential number). Would you please double-check whether this implementation is fine to use from extension side? Pdr@, Would you please take a look or introduce anyone who can review this CL? Thanks in advance.
pdr@chromium.org changed reviewers: + meade@chromium.org - pdr@chromium.org
pdr@chromium.org changed reviewers: + pdr@chromium.org
Eddy, could you please review this from the Blink side? https://crbug.com/608854 has an overview of the overall design.
On 2017/05/15 07:44:29, limasdf wrote: > Hi Devlin, Pdr. > > Devlin@, I took the option #1, Have blink return a non-unique > identifier(sequential number). Would you please double-check whether this > implementation is fine to use from extension side? This will work as long as we, on the extensions side, track unique_id_exposed_to_extension -> pair<blink_id, RenderFrameHost>. Since the id isn't globally unique, we'd need to expose something different to the extension. But that shouldn't be a problem, so this should work. (Note: I didn't look at the implementation much; leaving that to Eddy. :))
meade@chromium.org changed reviewers: + pdr@google.com, rune@opera.com - pdr@chromium.org
I'm not sure I'm the best reviewer for this area - Rune, could you PTAL? Thanks!
On 2017/05/16 07:15:17, meade_UTC10 wrote: > I'm not sure I'm the best reviewer for this area - Rune, could you PTAL? Thanks! Will do.
I have a concern about extensions being able to remove sheets injected by other extensions. I don't know what the code looks, or should look, like outside the web/ layer, but if we just simply pass the integer id out to the extension, it can remove all sheets inserted by previous extensions by removing 1..N-1 if it got the id N by inserting a sheet. Do you have a plan to make sure an extension can only remove sheets it inserted? https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StyleEngine.cpp (right): https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StyleEngine.cpp:141: AtomicIncrement(&injected_author_sheets_id_count_); StyleEngine is not made to be thread-safe. Why not just pre-increment the counter in the make_pair call? https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StyleEngine.cpp:152: for (size_t i = 0; i < injected_author_style_sheets_.size(); ++i) { I suppose we can assume the number of injected sheet is low enough to allow for O(n) removal. https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StyleEngine.h (right): https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StyleEngine.h:393: volatile int injected_author_sheets_id_count_ = 0; Why volatile? I would have used a plain unsigned instead. https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebDocument.h (left): https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/web/WebDocument.h:125: These API methods should have a unit test. https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebDocument.h (right): https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/web/WebDocument.h:125: BLINK_EXPORT int InsertStyleSheet(const WebString& source_code); Could you use a typedef with a suitable id type name instead? https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/web/WebDocument.h:130: BLINK_EXPORT bool RemoveInsertedStyleSheet(int id); Is a return value necessary here?
On 2017/05/16 09:00:59, rune wrote: > I have a concern about extensions being able to remove sheets injected by other > extensions. I don't know what the code looks, or should look, like outside the > web/ layer, but if we just simply pass the integer id out to the extension, it > can remove all sheets inserted by previous extensions by removing 1..N-1 if it > got the id N by inserting a sheet. > > Do you have a plan to make sure an extension can only remove sheets it inserted? Yep; this would be handled on the browser-side. We'd only let extensions remove stylesheets they themselves injected.
Description was changed from ========== Provide a method to remove inserted style sheet InsertStyleSheet() returns sequence id. And RemoveInsertedStyleSheet() removes the inserted style sheet with its id. TEST=webkit_unit_tests --gtest_fiter=StyleEngineTest.AnalyzedInject BUG=608854 ========== to ========== Provide a method to remove inserted style sheet InsertStyleSheet() returns sequence id. And RemoveInsertedStyleSheet() removes the inserted style sheet with its id. TEST=webkit_unit_tests --gtest_fiter=StyleEngineTest.AnalyzedInject, WebDocumentTest.InsertAndRemoveStyleSheet BUG=608854 ==========
Sorry for late. As Devlin mentioned above, The extension should remove CSS injected by the same extension. It is specified on API proposal[1]. PTAL. [1] https://docs.google.com/document/d/1rg88OKK88jGqG8Ez971vtjdpBkpuxpHgmQIRTsE89... https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StyleEngine.cpp (right): https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StyleEngine.cpp:141: AtomicIncrement(&injected_author_sheets_id_count_); On 2017/05/16 09:00:58, rune wrote: > StyleEngine is not made to be thread-safe. Why not just pre-increment the > counter in the make_pair call? Done. https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StyleEngine.h (right): https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StyleEngine.h:393: volatile int injected_author_sheets_id_count_ = 0; On 2017/05/16 09:00:58, rune wrote: > Why volatile? > > I would have used a plain unsigned instead. Done. https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebDocument.h (left): https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/web/WebDocument.h:125: On 2017/05/16 09:00:59, rune wrote: > These API methods should have a unit test. Done. https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebDocument.h (right): https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/web/WebDocument.h:125: BLINK_EXPORT int InsertStyleSheet(const WebString& source_code); On 2017/05/16 09:00:59, rune wrote: > Could you use a typedef with a suitable id type name instead? Done. https://codereview.chromium.org/2835183002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/web/WebDocument.h:130: BLINK_EXPORT bool RemoveInsertedStyleSheet(int id); On 2017/05/16 09:00:59, rune wrote: > Is a return value necessary here? I was thinking the extension api should return an error message when the user inserts wrong id. But it could be done by holding id from extension side. So, bool -> void (done).
lgtm if fixing comments below. You'll need a separate reviewer for web/ and public/ https://codereview.chromium.org/2835183002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StyleEngine.h (right): https://codereview.chromium.org/2835183002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StyleEngine.h:116: unsigned InjectAuthorSheet(StyleSheetContents* author_sheet); I think I would've used an stylesheet id type in this api as well. https://codereview.chromium.org/2835183002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebDocumentTest.cpp (right): https://codereview.chromium.org/2835183002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebDocumentTest.cpp:71: unsigned stylesheet_id = web_doc.InsertStyleSheet("body { color: green }"); WebStyleSheetId?
rune@, Thanks for the review! pdr@, Would you take a look at web/, public/ as the owner? https://codereview.chromium.org/2835183002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/StyleEngine.h (right): https://codereview.chromium.org/2835183002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/StyleEngine.h:116: unsigned InjectAuthorSheet(StyleSheetContents* author_sheet); On 2017/05/18 13:13:21, rune wrote: > I think I would've used an stylesheet id type in this api as well. > Done. https://codereview.chromium.org/2835183002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebDocumentTest.cpp (right): https://codereview.chromium.org/2835183002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebDocumentTest.cpp:71: unsigned stylesheet_id = web_doc.InsertStyleSheet("body { color: green }"); On 2017/05/18 13:13:21, rune wrote: > WebStyleSheetId? Done.
meade@chromium.org changed reviewers: - meade@chromium.org
Hi Pdr@, Sorry I need a separate reviewer for web/ and public/. Would you please take a look? or introduce another guy for review? Thanks in advance.
On 2017/05/23 at 08:49:05, limasdf wrote: > Hi Pdr@, > > Sorry I need a separate reviewer for web/ and public/. > Would you please take a look? or introduce another guy for review? > > Thanks in advance. LGTM
The CQ bit was checked by limasdf@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com Link to the patchset: https://codereview.chromium.org/2835183002/#ps300001 (title: "rebase")
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": 300001, "attempt_start_ts": 1495769405589970,
"parent_rev": "79f092fc8747729c369adc0ba85d6a124eba0d06", "commit_rev":
"0ffd86f3ca15a4728605fa70435297d334ccc35a"}
Message was sent while issue was closed.
Description was changed from ========== Provide a method to remove inserted style sheet InsertStyleSheet() returns sequence id. And RemoveInsertedStyleSheet() removes the inserted style sheet with its id. TEST=webkit_unit_tests --gtest_fiter=StyleEngineTest.AnalyzedInject, WebDocumentTest.InsertAndRemoveStyleSheet BUG=608854 ========== to ========== Provide a method to remove inserted style sheet InsertStyleSheet() returns sequence id. And RemoveInsertedStyleSheet() removes the inserted style sheet with its id. TEST=webkit_unit_tests --gtest_fiter=StyleEngineTest.AnalyzedInject, WebDocumentTest.InsertAndRemoveStyleSheet BUG=608854 Review-Url: https://codereview.chromium.org/2835183002 Cr-Commit-Position: refs/heads/master@{#474919} Committed: https://chromium.googlesource.com/chromium/src/+/0ffd86f3ca15a4728605fa704352... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:300001) as https://chromium.googlesource.com/chromium/src/+/0ffd86f3ca15a4728605fa704352... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
