Initial implementation of chrome.browser.openTab.

Initial implementation of chrome.browser.openTab.

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

[rpaquay, 2014-04-22 22:22:20]

miket@ for initial review.

[rpaquay, 2014-04-28 18:04:50]

(rebasing)

miket@: ping?

[miket_OOO, 2014-04-28 18:24:34]

LGTM, but I had trouble telling which code was new, and which was moved. Next
time if possible submit as two CLs, one just refactoring to move the code into
the new place, and the other with the new functionality.

Thanks!

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/browser/browser_apitest.cc (right):

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/browser/browser_apitest.cc:26: std::string url =
"about:blank";
Would a different URL be a better test? For example, a real http URL would
stress more of the system, adding an item to history, making sure other
profile-specific operations were OK, etc.

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/browser/browser_apitest.cc:38:
EXPECT_FALSE(result);
Is there a failure case? I see the IDL has a void callback. Under what
circumstances could this fail? (If no, that's OK; I just want to think about
it.)

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/extension_tab_util.cc:63: // |error_message| can
optionally be passed in a will be set with an appropriate
a => and?

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/extension_tab_util.cc:94: DCHECK(web_contents);
I don't know whether these should be DCHECKs or CHECKs. Could they fail in
production code?

https://codereview.chromium.org/245933002/diff/130001/chrome/common/extension...
File chrome/common/extensions/api/browser.idl (right):

https://codereview.chromium.org/245933002/diff/130001/chrome/common/extension...
chrome/common/extensions/api/browser.idl:7: namespace browser {
Let's nodoc this one for now.

https://codereview.chromium.org/245933002/diff/130001/chrome/common/extension...
chrome/common/extensions/api/browser.idl:10: // The URL to navigate to when the
new tab is initially open.
opened

https://codereview.chromium.org/245933002/diff/130001/chrome/common/extension...
chrome/common/extensions/api/browser.idl:19: // profile is open, a new one is
open prior to creating the new tab. The
"is opened"

[rpaquay, 2014-04-28 19:44:12]

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/api/browser/browser_apitest.cc (right):

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/browser/browser_apitest.cc:26: std::string url =
"about:blank";
On 2014/04/28 18:24:35, miket wrote:
> Would a different URL be a better test? For example, a real http URL would
> stress more of the system, adding an item to history, making sure other
> profile-specific operations were OK, etc.

Agreed. I only added a "sanity check" test at this point, which exercises
already as-is most of the code in this CL. I didn't want to start with a test
using an actual URL to avoid potential issues with networking, setting up a
local server, etc. I am planning to add more eventually, when/if we get specific
feedback about additional requirements for this API.

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/api/browser/browser_apitest.cc:38:
EXPECT_FALSE(result);
On 2014/04/28 18:24:35, miket wrote:
> Is there a failure case? I see the IDL has a void callback. Under what
> circumstances could this fail? (If no, that's OK; I just want to think about
> it.)

The failure cases would be reported through "chrome.runtime.lastError.message".
Currently, there are 2 I can see in the code path (re-used from
chrome.tabs.openTab): passing a "crash" url or an "invalid" url (according to
ExtensionTabUtil::ResolvePossiblyRelativeURL).

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/extension_tab_util.cc:63: // |error_message| can
optionally be passed in a will be set with an appropriate
On 2014/04/28 18:24:35, miket wrote:
> a => and?

Done.

https://codereview.chromium.org/245933002/diff/130001/chrome/browser/extensio...
chrome/browser/extensions/extension_tab_util.cc:94: DCHECK(web_contents);
On 2014/04/28 18:24:35, miket wrote:
> I don't know whether these should be DCHECKs or CHECKs. Could they fail in
> production code?

AFAIK, looking the the implementations, the first three DCHECKs should always be
verified.

The last DCHECK is more of an pre-condition check: the "CreateBrower" function
should be called only in this specific case (no browser opened for the profile).

https://codereview.chromium.org/245933002/diff/130001/chrome/common/extension...
File chrome/common/extensions/api/browser.idl (right):

https://codereview.chromium.org/245933002/diff/130001/chrome/common/extension...
chrome/common/extensions/api/browser.idl:7: namespace browser {
On 2014/04/28 18:24:35, miket wrote:
> Let's nodoc this one for now.

Done.

https://codereview.chromium.org/245933002/diff/130001/chrome/common/extension...
chrome/common/extensions/api/browser.idl:10: // The URL to navigate to when the
new tab is initially open.
On 2014/04/28 18:24:35, miket wrote:
> opened

Done.

https://codereview.chromium.org/245933002/diff/130001/chrome/common/extension...
chrome/common/extensions/api/browser.idl:19: // profile is open, a new one is
open prior to creating the new tab. The
On 2014/04/28 18:24:35, miket wrote:
> "is opened"

Done.

[rpaquay, 2014-04-28 19:45:45]

isherman@, please look at changes in

extensions/browser/extension_function_histogram_value.h
tools/metrics/histograms/histograms.xml

[Ilya Sherman, 2014-04-28 20:02:43]

histograms lgtm

[rpaquay, 2014-04-28 20:18:29]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-04-28 20:19:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/245933002/170001

[commit-bot: I haz the power, 2014-04-28 21:10:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 21:10:47]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[rpaquay, 2014-04-30 18:12:26]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-04-30 18:13:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/245933002/190001

[commit-bot: I haz the power, 2014-04-30 18:13:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 18:13:25]

Failed to apply patch for
extensions/browser/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file extensions/browser/extension_function_histogram_value.h
  Hunk #1 FAILED at 806.
  1 out of 1 hunk FAILED -- saving rejects to file
extensions/browser/extension_function_histogram_value.h.rej

Patch:       extensions/browser/extension_function_histogram_value.h
Index: extensions/browser/extension_function_histogram_value.h
diff --git a/extensions/browser/extension_function_histogram_value.h
b/extensions/browser/extension_function_histogram_value.h
index
63827b5e48e9cc18ea5b81b9093a882d8fe674fa..e3015085d5412f661b710160d479340ee0f3c861
100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -806,6 +806,7 @@ enum HistogramValue {
   BLUETOOTHSOCKET_SEND,
   BLUETOOTHSOCKET_GETINFO,
   BLUETOOTHSOCKET_GETSOCKETS,
+  BROWSER_OPENTAB,
   // Last entry: Add new entries above and ensure to update
   // tools/metrics/histograms/histograms/histograms.xml.
   ENUM_BOUNDARY

[rpaquay, 2014-04-30 20:30:09]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-04-30 20:31:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/245933002/210001

[commit-bot: I haz the power, 2014-04-30 21:32:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 21:32:17]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[rpaquay, 2014-05-01 15:58:21]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-05-01 15:58:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/245933002/210001

[commit-bot: I haz the power, 2014-05-01 16:59:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 16:59:50]

Failed to apply patch for
extensions/browser/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file extensions/browser/extension_function_histogram_value.h
  Hunk #1 FAILED at 808.
  1 out of 1 hunk FAILED -- saving rejects to file
extensions/browser/extension_function_histogram_value.h.rej

Patch:       extensions/browser/extension_function_histogram_value.h
Index: extensions/browser/extension_function_histogram_value.h
diff --git a/extensions/browser/extension_function_histogram_value.h
b/extensions/browser/extension_function_histogram_value.h
index
5486d779b4ac5ae32738b642b110db23ca737d5d..838263e01204f4e07c8c79055447faab4e04eb00
100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -808,6 +808,7 @@ enum HistogramValue {
   BLUETOOTHSOCKET_GETSOCKETS,
   WEBSTOREPRIVATE_SIGNINFUNCTION,
   SHELL_CREATEWINDOW,
+  BROWSER_OPENTAB,
   // Last entry: Add new entries above and ensure to update
   // tools/metrics/histograms/histograms/histograms.xml.
   ENUM_BOUNDARY

[rpaquay, 2014-05-01 19:34:11]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-05-01 19:34:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/245933002/230001

[commit-bot: I haz the power, 2014-05-01 21:06:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 21:06:10]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium

[rpaquay, 2014-05-01 21:13:04]

The CQ bit was checked by rpaquay@chromium.org

[commit-bot: I haz the power, 2014-05-01 21:13:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/245933002/230001

[commit-bot: I haz the power, 2014-05-02 00:34:13]

Change committed as 267697