Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
400343002:
Add default fonts for extensions (Closed)
|
Created:
6 years, 5 months ago by ericzeng Modified:
6 years, 5 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd default fonts for extensions.
This CL applies the default system font style to extension pages.
Extensions that specify their own fonts will still override these styles.
BUG=141703, 395319
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284597
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address kalman's comments #
Total comments: 1
Patch Set 3 : Revert silly changes #
Total comments: 9
Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : Fix bug that caused DeclarativeContentApiTest to fail #
Messages
Total messages: 23 (0 generated)
PTAL
https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:328: if (IsWithinPlatformApp() || IsWithinExtension()) { I don't think (looking at IsWithinExtension) this is quite right. If an extension hosts an iframe for some page, that page shouldn't get the style of the extension, but I think that's what will happen here. likewise I think that IsWithinPlatformApp isn't right here either (though it's unlikely to actually matter since apps can't host web content in-process). but we might as well fix it: const Extension* extension = extensions_->GetExtensionOrAppByURL( frame->document()->url()); if (extension) { ... } https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:340: .as_string(); each of these has multi-line bodies; use {}. though it'd be simpler to write int resource_id = IsWithinPlatformApp() ? IDR_PLATFORM_APP_CSS : IDR_EXTENSION_CSS; ... https://codereview.chromium.org/400343002/diff/1/extensions/renderer/resource... File extensions/renderer/resources/extension.css (right): https://codereview.chromium.org/400343002/diff/1/extensions/renderer/resource... extensions/renderer/resources/extension.css:12: } could you upload this CL with --no-detect-copies (I think that's the flag)
that's not quite right either, give me a sec...
the complexity of that check makes me realise we should be testing that. https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:328: if (IsWithinPlatformApp() || IsWithinExtension()) { > const Extension* extension = extensions_->GetExtensionOrAppByURL( > frame->document()->url()); > if (extension) { > ... > } The URL shouldn't come from frame->document()->url() actually, since that won't include about:blank URLs. Quite the corner case, but we should fix it: // Note: use GetEffectiveDocumentURL not just frame->document()->url() // so that this also injects the stylesheet on about:blank frames that // are hosted in the extension process. GURL effective_document_url = ScriptContext::GetEffectiveDocumentURL( frame, frame->docuemnt()->url(), true /* match_about_blank */);
https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:328: if (IsWithinPlatformApp() || IsWithinExtension()) { On 2014/07/19 00:19:24, kalman wrote: > > > const Extension* extension = extensions_->GetExtensionOrAppByURL( > > frame->document()->url()); > > if (extension) { > > ... > > } > > The URL shouldn't come from frame->document()->url() actually, since that won't > include about:blank URLs. Quite the corner case, but we should fix it: > > // Note: use GetEffectiveDocumentURL not just frame->document()->url() > // so that this also injects the stylesheet on about:blank frames that > // are hosted in the extension process. > GURL effective_document_url = ScriptContext::GetEffectiveDocumentURL( > frame, frame->docuemnt()->url(), true /* match_about_blank */); Done. https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:340: .as_string(); On 2014/07/19 00:08:10, kalman wrote: > each of these has multi-line bodies; use {}. > > though it'd be simpler to write > > int resource_id = IsWithinPlatformApp() ? > IDR_PLATFORM_APP_CSS : IDR_EXTENSION_CSS; > ... Is there a case where the renderer could host something other than an extension or platform app? https://codereview.chromium.org/400343002/diff/1/extensions/renderer/resource... File extensions/renderer/resources/extension.css (right): https://codereview.chromium.org/400343002/diff/1/extensions/renderer/resource... extensions/renderer/resources/extension.css:12: } On 2014/07/19 00:08:10, kalman wrote: > could you upload this CL with --no-detect-copies (I think that's the flag) Done.
https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... extensions/renderer/dispatcher.cc:340: .as_string(); On 2014/07/19 01:48:59, Eric Zeng wrote: > On 2014/07/19 00:08:10, kalman wrote: > > each of these has multi-line bodies; use {}. > > > > though it'd be simpler to write > > > > int resource_id = IsWithinPlatformApp() ? > > IDR_PLATFORM_APP_CSS : IDR_EXTENSION_CSS; > > ... > > Is there a case where the renderer could host something other than an extension > or platform app? oh, right. yes :) good point. https://codereview.chromium.org/400343002/diff/20001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/400343002/diff/20001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:1066: bool Dispatcher::IsWithinPlatformApp(blink::WebFrame* frame) { sorry I didn't meant to imply that we modify IsWithinPlatformApp[*]. I meant that we shouldn't use it in DidCreateDocumentElement, and we shouldn't even need to add IsWithinExtension. [*] though it *does* look suspicious.
(the first patchset was more right; it should be a pretty small delta in dispatcher.cc, maybe 10 lines)
On 2014/07/19 01:54:55, kalman wrote: > (the first patchset was more right; it should be a pretty small delta in > dispatcher.cc, maybe 10 lines) Should be fixed now
awesome. just nits left. you should also add bug 141703 to the BUG line. https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:335: if (extension) { given the whole rest of this method is inside this if-block, perhaps reverse the condition and early-return instead. https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:341: // are loaded in each app. this comment belongs before the "insertStyleSheet" call. https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:343: .GetRawDataResource(IDR_PLATFORM_APP_CSS) it would be simpler to switch on the resource ID rather than the resource. then you can also use ternary and this block becomes even simpler. https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:355: content_watcher_->DidCreateDocumentElement(frame); (note: moving this inside the if-block is a change in behaviour, though it's more correct now, so... cool)
https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:335: if (extension) { On 2014/07/21 16:44:23, kalman wrote: > given the whole rest of this method is inside this if-block, perhaps reverse the > condition and early-return instead. Done. https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:341: // are loaded in each app. On 2014/07/21 16:44:23, kalman wrote: > this comment belongs before the "insertStyleSheet" call. Done. https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:343: .GetRawDataResource(IDR_PLATFORM_APP_CSS) On 2014/07/21 16:44:23, kalman wrote: > it would be simpler to switch on the resource ID rather than the resource. then > you can also use ternary and this block becomes even simpler. What do you mean by switching on the resource ID? No ternary operator! https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch...
https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:343: .GetRawDataResource(IDR_PLATFORM_APP_CSS) On 2014/07/21 18:12:33, Eric Zeng wrote: > On 2014/07/21 16:44:23, kalman wrote: > > it would be simpler to switch on the resource ID rather than the resource. > then > > you can also use ternary and this block becomes even simpler. > > What do you mean by switching on the resource ID? > > No ternary operator! > https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... Ternary operator is fine here because the block is guarded on (extension->is_extension() || extension->is_platform_app()). and "by switching on the resource ID" I mean, basically the code I wrote in that comment you just linked to.
https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/400343002/diff/40001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:343: .GetRawDataResource(IDR_PLATFORM_APP_CSS) On 2014/07/21 18:31:29, kalman wrote: > On 2014/07/21 18:12:33, Eric Zeng wrote: > > On 2014/07/21 16:44:23, kalman wrote: > > > it would be simpler to switch on the resource ID rather than the resource. > > then > > > you can also use ternary and this block becomes even simpler. > > > > What do you mean by switching on the resource ID? > > > > No ternary operator! > > > https://codereview.chromium.org/400343002/diff/1/extensions/renderer/dispatch... > > Ternary operator is fine here because the block is guarded on > (extension->is_extension() || extension->is_platform_app()). > > and "by switching on the resource ID" I mean, basically the code I wrote in that > comment you just linked to. Got it, but I'll use extension->is_platform_app() rather than IsWithinPlatformApp().
lgtm https://codereview.chromium.org/400343002/diff/80001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/400343002/diff/80001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:349: // WebKit doesn't let us define an additional user agent stylesheet, so s/WebKit/Blink/
The CQ bit was checked by ericzeng@chromium.org
The CQ bit was checked by ericzeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericzeng@chromium.org/400343002/80001
The CQ bit was unchecked by ericzeng@chromium.org
https://codereview.chromium.org/400343002/diff/80001/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/400343002/diff/80001/extensions/renderer/disp... extensions/renderer/dispatcher.cc:349: // WebKit doesn't let us define an additional user agent stylesheet, so On 2014/07/21 18:48:19, kalman wrote: > s/WebKit/Blink/ Done.
The CQ bit was checked by ericzeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericzeng@chromium.org/400343002/100001
The CQ bit was checked by ericzeng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericzeng@chromium.org/400343002/120001
Message was sent while issue was closed.
Change committed as 284597 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
