<webview>: Move back, forward, canGoBack, canGoForward, go from content to chrome

<webview>: Move back, forward, canGoBack, canGoForward, go from content to chrome

BUG=166165
Test=WebViewTest.Navigation

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208642

[Fady Samuel, 2013-06-21 19:12:18]

Hi Istiaque,

With the other two patches LGTM'ed, this patch is now ready for review. Thanks!

Fady

[lazyboy, 2013-06-22 04:29:14]

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/browser/exte...
File chrome/browser/extensions/api/webview/webview_api.h (right):

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/webview/webview_api.h:63: virtual bool RunImpl()
OVERRIDE;
private: DISALLOW_COPY_AND_ASSIGN
Seems other function(s) are missing this too.

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/browser/exte...
File chrome/browser/extensions/api/webview/webview_api_constants.h (right):

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/webview/webview_api_constants.h:16: extern const
char kCurrentEntryIndex[];
These two are for internal event dispatched to plugin but not to <webview>, so
probably better to group them separately.

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/browser/exte...
File chrome/browser/extensions/web_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/browser/exte...
chrome/browser/extensions/web_view_browsertest.cc:368:
base::StringPrintf("Done%sTest.PASSED", test_type.c_str()), false);
Instead of sending test_type, i'd suggest passing test_passed_msg and
test_failed_msg and send "DoneGeolocationTest.PASSED" and
"DoneGeolocationTest.FAILED" respectively, otherwise not obvious to figure this
out looking at JavaScript code. Easier to grep too.

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/renderer/res...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/renderer/res...
chrome/renderer/resources/extensions/web_view.js:133: return self.entryCount_ >
1 && self.currentEntryIndex_ > 0;
generally for member vars, declare them in prototype:

/** @type {number} */
WebView.prototype.entryCount_;

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/renderer/res...
chrome/renderer/resources/extensions/web_view.js:138: self.currentEntryIndex_ <
(self.entryCount_ -1);
nit: space before 1

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/renderer/res...
chrome/renderer/resources/extensions/web_view.js:145:
chrome.webview.go(instanceId, -1);
nit: space before 1

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/test/data/ex...
File chrome/test/data/extensions/platform_apps/web_view/navigation/embedder.js
(right):

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/test/data/ex...
chrome/test/data/extensions/platform_apps/web_view/navigation/embedder.js:191:
default:
log: 'unexpected step: ' + step

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/test/data/ex...
File chrome/test/data/extensions/platform_apps/web_view/navigation/guest.html
(right):

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/test/data/ex...
chrome/test/data/extensions/platform_apps/web_view/navigation/guest.html:9: // A
guest that monitors focus and edit commands.
Update.

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/test/data/ex...
chrome/test/data/extensions/platform_apps/web_view/navigation/guest.html:30: if
(data[0] == 'select-all') {
Clean this and stuffs below.

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/test/data/ex...
File chrome/test/data/extensions/platform_apps/web_view/navigation/manifest.json
(right):

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/test/data/ex...
chrome/test/data/extensions/platform_apps/web_view/navigation/manifest.json:3:
"description": "Loads a guest which monitors focus",
Fix.

[Fady Samuel, 2013-06-25 15:46:12]

PTAL

https://codereview.chromium.org/17447005/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/webview/webview_api.h (right):

https://codereview.chromium.org/17447005/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/webview/webview_api.h:63: virtual bool RunImpl()
OVERRIDE;
On 2013/06/22 04:29:15, lazyboy wrote:
> private: DISALLOW_COPY_AND_ASSIGN
> Seems other function(s) are missing this too.
chrome/browser/extensions/extension_function_registry.h:22:14: error: no
matching constructor for initialization of 'WebviewExecuteScriptFunction'

Looks like ExtensionFunctionRegistry uses the default constructor. I'm leaving
it as is.

https://codereview.chromium.org/17447005/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/webview/webview_api_constants.h (right):

https://codereview.chromium.org/17447005/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/webview/webview_api_constants.h:16: extern const
char kCurrentEntryIndex[];
On 2013/06/22 04:29:15, lazyboy wrote:
> These two are for internal event dispatched to plugin but not to <webview>, so
> probably better to group them separately.

Well, they are dispatched to the webview shim but not to the developer facing
API, but yes. Done.

https://codereview.chromium.org/17447005/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/web_view_browsertest.cc (right):

https://codereview.chromium.org/17447005/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:368:
base::StringPrintf("Done%sTest.PASSED", test_type.c_str()), false);
On 2013/06/22 04:29:15, lazyboy wrote:
> Instead of sending test_type, i'd suggest passing test_passed_msg and
> test_failed_msg and send "DoneGeolocationTest.PASSED" and
> "DoneGeolocationTest.FAILED" respectively, otherwise not obvious to figure
this
> out looking at JavaScript code. Easier to grep too.

Done.

https://codereview.chromium.org/17447005/diff/4001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/17447005/diff/4001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:133: return self.entryCount_ >
1 && self.currentEntryIndex_ > 0;
On 2013/06/22 04:29:15, lazyboy wrote:
> generally for member vars, declare them in prototype:
> 
> /** @type {number} */
> WebView.prototype.entryCount_;

Done.

https://codereview.chromium.org/17447005/diff/4001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:138: self.currentEntryIndex_ <
(self.entryCount_ -1);
On 2013/06/22 04:29:15, lazyboy wrote:
> nit: space before 1

Done.

https://codereview.chromium.org/17447005/diff/4001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:145:
chrome.webview.go(instanceId, -1);
On 2013/06/22 04:29:15, lazyboy wrote:
> nit: space before 1

Really? This is a unary operator, not a binary operator in this context. Leaving
as is.

https://codereview.chromium.org/17447005/diff/4001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/web_view/navigation/embedder.js
(right):

https://codereview.chromium.org/17447005/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view/navigation/embedder.js:191:
default:
On 2013/06/22 04:29:15, lazyboy wrote:
> log: 'unexpected step: ' + step

Done.

https://codereview.chromium.org/17447005/diff/4001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/web_view/navigation/guest.html
(right):

https://codereview.chromium.org/17447005/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view/navigation/guest.html:9: // A
guest that monitors focus and edit commands.
On 2013/06/22 04:29:15, lazyboy wrote:
> Update.

Removing file.

https://codereview.chromium.org/17447005/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view/navigation/guest.html:30: if
(data[0] == 'select-all') {
On 2013/06/22 04:29:15, lazyboy wrote:
> Clean this and stuffs below.

Removing file.

https://codereview.chromium.org/17447005/diff/4001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/web_view/navigation/manifest.json
(right):

https://codereview.chromium.org/17447005/diff/4001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view/navigation/manifest.json:3:
"description": "Loads a guest which monitors focus",
On 2013/06/22 04:29:15, lazyboy wrote:
> Fix.

Done.

[lazyboy, 2013-06-25 15:58:49]

lgtm

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/browser/exte...
File chrome/browser/extensions/api/webview/webview_api.h (right):

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/browser/exte...
chrome/browser/extensions/api/webview/webview_api.h:63: virtual bool RunImpl()
OVERRIDE;
On 2013/06/25 15:46:12, Fady Samuel wrote:
> On 2013/06/22 04:29:15, lazyboy wrote:
> > private: DISALLOW_COPY_AND_ASSIGN
> > Seems other function(s) are missing this too.
> chrome/browser/extensions/extension_function_registry.h:22:14: error: no
> matching constructor for initialization of 'WebviewExecuteScriptFunction'
> 
> Looks like ExtensionFunctionRegistry uses the default constructor. I'm leaving
> it as is.
OK, I'd add a comment that says DISALLOW_COPY_AND_ASSIGN is intentionally left
out.

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/renderer/res...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/17447005/diff/4001/chrome/renderer/res...
chrome/renderer/resources/extensions/web_view.js:145:
chrome.webview.go(instanceId, -1);
On 2013/06/25 15:46:12, Fady Samuel wrote:
> On 2013/06/22 04:29:15, lazyboy wrote:
> > nit: space before 1
> 
> Really? This is a unary operator, not a binary operator in this context.
Leaving
> as is.

I misread the line 0_0

[Fady Samuel, 2013-06-25 19:22:05]

+mpcomplete@ for chrome/browser/extensions/* review.

https://codereview.chromium.org/17447005/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/webview/webview_api.h (right):

https://codereview.chromium.org/17447005/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/webview/webview_api.h:63: virtual bool RunImpl()
OVERRIDE;
On 2013/06/25 15:58:50, lazyboy wrote:
> On 2013/06/25 15:46:12, Fady Samuel wrote:
> > On 2013/06/22 04:29:15, lazyboy wrote:
> > > private: DISALLOW_COPY_AND_ASSIGN
> > > Seems other function(s) are missing this too.
> > chrome/browser/extensions/extension_function_registry.h:22:14: error: no
> > matching constructor for initialization of 'WebviewExecuteScriptFunction'
> > 
> > Looks like ExtensionFunctionRegistry uses the default constructor. I'm
leaving
> > it as is.
> OK, I'd add a comment that says DISALLOW_COPY_AND_ASSIGN is intentionally left
> out.

Looks like I was wrong. DISALLOW_COPY_AND_ASSIGN works, but I needed to add a
constructor. Done that.

[Matt Perry, 2013-06-25 19:33:12]

lgtm

https://codereview.chromium.org/17447005/diff/39001/chrome/browser/extensions...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/17447005/diff/39001/chrome/browser/extensions...
chrome/browser/extensions/api/webview/webview_api.cc:103:
render_view_host()->GetProcess()->GetID(), instance_id);
indent to 4 when wrapping

https://codereview.chromium.org/17447005/diff/39001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/17447005/diff/39001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view.js:154: return self.entryCount_ >
1 && self.currentEntryIndex_ > 0;
I'm not familiar enough with how this works, but I'm assuming that entryCount_
and currentEntryIndex_ can't change behind the scenes (e.g. the browser
navigates but doesn't inform the renderer in time)?

[Fady Samuel, 2013-06-25 21:52:44]

+cdn@ to sign off on browser_plugin_messages change.

https://codereview.chromium.org/17447005/diff/39001/chrome/browser/extensions...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/17447005/diff/39001/chrome/browser/extensions...
chrome/browser/extensions/api/webview/webview_api.cc:103:
render_view_host()->GetProcess()->GetID(), instance_id);
On 2013/06/25 19:33:12, Matt Perry wrote:
> indent to 4 when wrapping

Done.

https://codereview.chromium.org/17447005/diff/39001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/17447005/diff/39001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view.js:154: return self.entryCount_ >
1 && self.currentEntryIndex_ > 0;
On 2013/06/25 19:33:12, Matt Perry wrote:
> I'm not familiar enough with how this works, but I'm assuming that entryCount_
> and currentEntryIndex_ can't change behind the scenes (e.g. the browser
> navigates but doesn't inform the renderer in time)?

Nope.

[Cris Neckar, 2013-06-25 23:07:56]

ipc audit rubberstamp lgtm

[commit-bot: I haz the power, 2013-06-25 23:08:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/17447005/41001

[commit-bot: I haz the power, 2013-06-26 06:03:43]

Change committed as 208642