Issue 445403002: [android_webview] Factor AwWebContentsObserver out of AwContentsClient.

Issue 445403002: [android_webview] Factor AwWebContentsObserver out of AwContentsClient. (Closed)

Created:
6 years, 4 months ago by mkosiba (inactive)

Modified:
6 years, 4 months ago

Reviewers:
boliu

CC:
chromium-reviews, android-webview-reviews_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Project:
chromium

Visibility:
Public.

More Reviews

Description

[android_webview] Factor AwWebContentsObserver out of AwContentsClient. This moves AwWebContentsObserver to be a separate class so that it can has it's own unittests. BUG=None TEST=AndroidWebviewTests Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288376

Patch Set 1 #

Total comments: 8

Patch Set 2 : #

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+274 lines, -105 lines)			Patch
M	android_webview/java/src/org/chromium/android_webview/AwContents.java	View		4 chunks	+11 lines, -1 line	0 comments	Download
M	android_webview/java/src/org/chromium/android_webview/AwContentsClient.java	View		3 chunks	+0 lines, -68 lines	0 comments	Download
A	android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java	View		1 chunk	+70 lines, -0 lines	0 comments	Download
M	android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientVisitedHistoryTest.java	View	1	3 chunks	+6 lines, -36 lines	0 comments	Download
A	android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java	View	1	1 chunk	+151 lines, -0 lines	0 comments	Download
M	android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java	View		4 chunks	+36 lines, -0 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

boliu

Mostly nits https://codereview.chromium.org/445403002/diff/1/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientVisitedHistoryTest.java File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientVisitedHistoryTest.java (right): https://codereview.chromium.org/445403002/diff/1/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientVisitedHistoryTest.java#newcode41 android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientVisitedHistoryTest.java:41: private static class TestAwContentsClient Can we name ...

6 years, 4 months ago (2014-08-07 16:01:29 UTC) #2

mkosiba (inactive)

https://codereview.chromium.org/445403002/diff/1/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientVisitedHistoryTest.java File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientVisitedHistoryTest.java (right): https://codereview.chromium.org/445403002/diff/1/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientVisitedHistoryTest.java#newcode41 android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientVisitedHistoryTest.java:41: private static class TestAwContentsClient On 2014/08/07 16:01:29, boliu wrote: ...

6 years, 4 months ago (2014-08-08 10:58:44 UTC) #3

https://codereview.chromium.org/445403002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientVisitedHistoryTest.java
(right):

https://codereview.chromium.org/445403002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientVisitedHistoryTest.java:41:
private static class TestAwContentsClient
On 2014/08/07 16:01:29, boliu wrote:
> Can we name this VisitedHistoryTestAwContentsClient to avoid confusion?

Done.

https://codereview.chromium.org/445403002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java
(right):

https://codereview.chromium.org/445403002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java:33:
getInstrumentation().runOnMainSync(new Runnable() {
On 2014/08/07 16:01:29, boliu wrote:
> Is runOnMainSync here necessary? Test won't start running until setUp returns,
> right?

The constructor DCHECKS when it's not run on the UI thread. Added comment.

https://codereview.chromium.org/445403002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java:47:
mWebContentsObserver.didFinishLoad(/*frameId=*/0, EXAMPLE_URL,
/*isMainFrame=*/false);
On 2014/08/07 16:01:29, boliu wrote:
> nit: Not sure what the java style is, but in chromium c++, I've seen this kind
> of comments written as
> 
> didFinishLoad(0 /* frameId */, EXAMPLE_URL, false /* isMainFrame */);
> 
> So more spacing, and after the variable.
> 
> Maybe a clearer way is to declare local variable frameId and isMainFrame
instead
> of constants.

Done.

https://codereview.chromium.org/445403002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwWebContentsObserverTest.java:124:
mContentsClient.getDoUpdateVisitedHistoryHelper().getCallCount());
On 2014/08/07 16:01:29, boliu wrote:
> check isReload matches

Done.

mkosiba (inactive)

The CQ bit was checked by mkosiba@chromium.org

6 years, 4 months ago (2014-08-08 15:20:31 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mkosiba@chromium.org/445403002/20001

6 years, 4 months ago (2014-08-08 15:21:11 UTC) #6

Message was sent while issue was closed.

Change committed as 288376

Expand Messages | Collapse Messages