Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12496015:
Make sure only platform app support <webview> tag (Closed)
|
Created:
7 years, 9 months ago by Yang Gu Modified:
7 years, 9 months ago Reviewers:
Finnur, fsamuel, mthiesse, rafaelw, jeremya, Fady Samuel, Charlie Reis, PhistucK, not at google - send to devlin CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org, yupingx.chen_intel.com Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionMake sure only platform app support <webview> tag
To support <webview> tag, the DOM modification will slow down, but
<webview> tag is only meaningful for platform app.
Contributed by yupingx.chen@intel.com
BUG=http://crbug.com/196453
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189235
Patch Set 1 : #
Total comments: 3
Patch Set 2 : Modify comments and cache the result of isWithinPlatformApp(frame) #
Total comments: 1
Patch Set 3 : indentation issue #
Total comments: 1
Patch Set 4 : Set is_within_platform_app to IsWithinPlatformApp(frame) #Messages
Total messages: 18 (0 generated)
Hi all, Thank you in advance.
lgtm
LGTM with fixes for the typos. https://codereview.chromium.org/12496015/diff/3001/chrome/renderer/extensions... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/12496015/diff/3001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:984: // Only platform app need to support <webview> tag, "webView" and Please fix typos: Only platform apps support the <webview> tag, because the "webView" and https://codereview.chromium.org/12496015/diff/3001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:985: // "denyWebView" modules will effect the performace of the DOM modification nit: will effect -> affect nit: performance of DOM modifications
https://codereview.chromium.org/12496015/diff/3001/chrome/renderer/extensions... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/12496015/diff/3001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:988: IsWithinPlatformApp(frame)) { Just driving by - Maybe cache the result of IsWithinPlatformApp(frame) in a variable and save a microsecond? It is called a couple of lines prior to this line as well. (I am not a C++ guy)
On 2013/03/18 15:16:05, creis wrote: > LGTM with fixes for the typos. > > https://codereview.chromium.org/12496015/diff/3001/chrome/renderer/extensions... > File chrome/renderer/extensions/dispatcher.cc (right): > > https://codereview.chromium.org/12496015/diff/3001/chrome/renderer/extensions... > chrome/renderer/extensions/dispatcher.cc:984: // Only platform app need to > support <webview> tag, "webView" and > Please fix typos: > Only platform apps support the <webview> tag, because the "webView" and > Done. > https://codereview.chromium.org/12496015/diff/3001/chrome/renderer/extensions... > chrome/renderer/extensions/dispatcher.cc:985: // "denyWebView" modules will > effect the performace of the DOM modification > nit: will effect -> affect > nit: performance of DOM modifications Done.
On 2013/03/18 17:50:41, PhistucK wrote: > https://codereview.chromium.org/12496015/diff/3001/chrome/renderer/extensions... > File chrome/renderer/extensions/dispatcher.cc (right): > > https://codereview.chromium.org/12496015/diff/3001/chrome/renderer/extensions... > chrome/renderer/extensions/dispatcher.cc:988: IsWithinPlatformApp(frame)) { > Just driving by - > Maybe cache the result of IsWithinPlatformApp(frame) in a variable and save a > microsecond? It is called a couple of lines prior to this line as well. > > (I am not a C++ guy) Done.
https://codereview.chromium.org/12496015/diff/8001/chrome/renderer/extensions... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/12496015/diff/8001/chrome/renderer/extensions... chrome/renderer/extensions/dispatcher.cc:989: is_within_platform_app) { Drive-by nit: indentation is off.
On 2013/03/19 18:45:28, mthiesse wrote: > https://codereview.chromium.org/12496015/diff/8001/chrome/renderer/extensions... > File chrome/renderer/extensions/dispatcher.cc (right): > > https://codereview.chromium.org/12496015/diff/8001/chrome/renderer/extensions... > chrome/renderer/extensions/dispatcher.cc:989: is_within_platform_app) { > Drive-by nit: indentation is off. Done.
https://codereview.chromium.org/12496015/diff/14001/chrome/renderer/extension... File chrome/renderer/extensions/dispatcher.cc (right): https://codereview.chromium.org/12496015/diff/14001/chrome/renderer/extension... chrome/renderer/extensions/dispatcher.cc:980: bool is_within_platform_app = extension && extension->is_platform_app(); Wait, you're changing behavior here. We're using different frames to determine the extension in this approach vs. calling IsWithinPlatformApp. IsWithinPlatformApp looks at frame->top, while this extension comes from frame itself. I don't understand the context well enough to know if that's always safe. Why change it? Is there a reason it's better?
On 2013/03/20 01:52:25, creis wrote: > https://codereview.chromium.org/12496015/diff/14001/chrome/renderer/extension... > File chrome/renderer/extensions/dispatcher.cc (right): > > https://codereview.chromium.org/12496015/diff/14001/chrome/renderer/extension... > chrome/renderer/extensions/dispatcher.cc:980: bool is_within_platform_app = > extension && extension->is_platform_app(); > Wait, you're changing behavior here. We're using different frames to determine > the extension in this approach vs. calling IsWithinPlatformApp. > IsWithinPlatformApp looks at frame->top, while this extension comes from frame > itself. > > I don't understand the context well enough to know if that's always safe. Why > change it? Is there a reason it's better? Yes, you are right. I change it back. Done.
Thanks. LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yang.gu@intel.com/12496015/18001
Presubmit check for 12496015-18001 failed and returned exit status 1.
INFO:root:Found 1 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/renderer/extensions/dispatcher.cc
Presubmit checks took 1.2s to calculate.
Hi kalman, jeremya, Finnur, Could you please take a second to review the patch? Thank you.
lgtm :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yang.gu@intel.com/12496015/18001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yang.gu@intel.com/12496015/18001
Message was sent while issue was closed.
Change committed as 189235 |
