Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(528)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 216983003: Chrome side changes to removing WebFrame parameter from WebPermissionClient, since it's redundant n… (Closed)

Created:
6 years, 8 months ago by jam
Modified:
6 years, 8 months ago
Reviewers:
nasko
CC:
chromium-reviews, darin-cc_chromium.org, android-webview-reviews_chromium.org, jochen+watch_chromium.org, site-isolation-reviews_chromium.org
Visibility:
Public.

Description

Chrome side changes to removing WebFrame parameter from WebPermissionClient, since it's redundant now. BUG=304341 R=nasko@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261136

Patch Set 1 #

Patch Set 2 : #

Total comments: 2

Patch Set 3 : review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+222 lines, -33 lines) Patch
M android_webview/renderer/aw_permission_client.h View 2 chunks +10 lines, -0 lines 0 comments Download
M android_webview/renderer/aw_permission_client.cc View 2 chunks +15 lines, -1 line 0 comments Download
M chrome/renderer/content_settings_observer.h View 1 2 2 chunks +30 lines, -0 lines 0 comments Download
M chrome/renderer/content_settings_observer.cc View 1 2 14 chunks +124 lines, -27 lines 0 comments Download
M content/shell/renderer/test_runner/WebPermissions.h View 2 chunks +8 lines, -0 lines 0 comments Download
M content/shell/renderer/test_runner/WebPermissions.cpp View 1 2 chunks +35 lines, -5 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
jam
6 years, 8 months ago (2014-04-01 17:21:57 UTC) #1
nasko
LGTM with a nit. https://codereview.chromium.org/216983003/diff/20001/chrome/renderer/content_settings_observer.h File chrome/renderer/content_settings_observer.h (right): https://codereview.chromium.org/216983003/diff/20001/chrome/renderer/content_settings_observer.h#newcode108 chrome/renderer/content_settings_observer.h:108: virtual bool allowWebComponents(bool); nit: shouldn't ...
6 years, 8 months ago (2014-04-01 20:14:30 UTC) #2
jam
https://codereview.chromium.org/216983003/diff/20001/chrome/renderer/content_settings_observer.h File chrome/renderer/content_settings_observer.h (right): https://codereview.chromium.org/216983003/diff/20001/chrome/renderer/content_settings_observer.h#newcode108 chrome/renderer/content_settings_observer.h:108: virtual bool allowWebComponents(bool); On 2014/04/01 20:14:30, nasko wrote: > ...
6 years, 8 months ago (2014-04-01 21:35:09 UTC) #3
bungeman-skia
On 2014/04/01 21:35:09, jam wrote: > https://codereview.chromium.org/216983003/diff/20001/chrome/renderer/content_settings_observer.h > File chrome/renderer/content_settings_observer.h (right): > > https://codereview.chromium.org/216983003/diff/20001/chrome/renderer/content_settings_observer.h#newcode108 > ...
6 years, 8 months ago (2014-04-02 17:56:04 UTC) #4
jam
6 years, 8 months ago (2014-04-02 18:04:39 UTC) #5
Message was sent while issue was closed. 
On 2014/04/02 17:56:04, bungeman1 wrote:
> On 2014/04/01 21:35:09, jam wrote:
> >
>
https://codereview.chromium.org/216983003/diff/20001/chrome/renderer/content_...
> > File chrome/renderer/content_settings_observer.h (right):
> > 
> >
>
https://codereview.chromium.org/216983003/diff/20001/chrome/renderer/content_...
> > chrome/renderer/content_settings_observer.h:108: virtual bool
> > allowWebComponents(bool);
> > On 2014/04/01 20:14:30, nasko wrote:
> > > nit: shouldn't the parameter be named for consistency with the rest?
> > 
> > yep, done. I had just copied it :)
> 
> This may have caused a test failure (See
>
http://build.chromium.org/p/chromium.linux/buildstatus?builder=Android%20Test...
> )
> 
> C  174s Main  [FAIL]
>
org.chromium.android_webview.test.AwContentsClientFullScreenVideoTest#testOnShowAndHideCustomView:
> C  174s Main  java.lang.SecurityException: Injecting to another application
> requires INJECT_EVENTS permission
> C  174s Main  	at android.os.Parcel.readException(Parcel.java:1465)
> C  174s Main  	at android.os.Parcel.readException(Parcel.java:1419)
> C  174s Main  	at
>
android.hardware.input.IInputManager$Stub$Proxy.injectInputEvent(IInputManager.java:356)
> C  174s Main  	at
> android.hardware.input.InputManager.injectInputEvent(InputManager.java:641)
> C  174s Main  	at
> android.app.Instrumentation.sendKeySync(Instrumentation.java:894)
> C  174s Main  	at
> android.app.Instrumentation.sendKeyDownUpSync(Instrumentation.java:904)
> C  174s Main  	at
>
org.chromium.android_webview.test.AwContentsClientFullScreenVideoTest.testOnShowAndHideCustomView(AwContentsClientFullScreenVideoTest.java:37)
> C  174s Main  	at java.lang.reflect.Method.invokeNative(Native Method)
> C  174s Main  	at
>
android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
> C  174s Main  	at
> android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
> C  174s Main  	at
>
android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
> C  174s Main  	at
> android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191)
> C  174s Main  	at
> android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176)
> C  174s Main  	at
>
android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554)
> C  174s Main  	at
>
android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701)

that test appears to be flaky, i.e. see it failed earlier:
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...

can you disable it?

Powered by Google App Engine
This is Rietveld 408576698