Move extensions code from WebContents::GetURL to GetLastCommittedURL.

Move extensions code from WebContents::GetURL to GetLastCommittedURL.

WebContents::GetURL has been deprecated for a while and is hard to use
correctly. This CL is moving some extensions code to use GetLastCommittedURL
instead, which is the proper replacement.

BUG=237908

[nasko, 2016-10-19 18:56:07]

The CQ bit was checked by nasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 18:57:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 19:50:08]

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

[commit-bot: I haz the power, 2016-10-19 19:50:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nasko, 2016-10-19 21:46:00]

The CQ bit was checked by nasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 21:46:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 22:26:23]

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

[commit-bot: I haz the power, 2016-10-19 22:26:24]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nasko, 2016-10-20 00:46:57]

Description was changed from

==========
Move extensions code from WebContents::GetURL to GetLastCommittedURL.

WebContents::GetURL has been deprecated for a while and is hard to use
correctly. This CL is moving extensions code to use GetLastCommittedURL
instead, which is the proper replacement.

BUG=237908
==========

to

==========
Move extensions code from WebContents::GetURL to GetLastCommittedURL.

WebContents::GetURL has been deprecated for a while and is hard to use
correctly. This CL is moving some extensions code to use GetLastCommittedURL
instead, which is the proper replacement.

BUG=237908
==========

[nasko, 2016-10-20 00:48:41]

nasko@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[nasko, 2016-10-20 00:48:43]

Hey Devlin,
How does this look for first stab at moving Tab object creation to using
WC::GetLastCommittedURL?
I've left only the chrome.tabs.update() callback to use the visible URL, as this
is the expected return value and we discussed that it will otherwise lead to
confusing API.
Thanks in advance!
Nasko

https://codereview.chromium.org/2434063002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_tab_util.h (right):

https://codereview.chromium.org/2434063002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_tab_util.h:87: enum UrlType { LastCommitted,
Visible };
This along with the methods taking it can potentially move into the anonymous
namespace in the .cc file. I did hit some linker errors when I tried that, but
didn't dig further.

[nasko, 2016-10-20 00:48:49]

The CQ bit was checked by nasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 00:49:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 03:05:33]

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

[commit-bot: I haz the power, 2016-10-20 03:05:34]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-10-20 17:27:24]

Two questions:
- How does this not break the test where we create a tab and expect the url to
be what we just updated it to in the test? [1]  That is,
chrome.tabs.create({url: 'example.com'}, function(tab) {
  assertEq('example.com', tab.url);
});

- We return the visible url in the response to update(), but not in the
onUpdated method.  Does that mean that if I update a tab and listen to
onUpdated, the tabs passed are different?  e.g. let's say we start on
example.com:

chrome.tabs.onUpdated.addListener(function(tab) {
  tab.url  // Is this the last committed url, which would be example.com?
});

chrome.tabs.update({tabId: 1, url: 'google.com'}, function(tab) {
  tab.url  // Is this the last visible url, which would be google.com?
});

The mismatch between those two seems rather odd... :/

[1]
https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/tab...

https://codereview.chromium.org/2434063002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_tab_util.h (right):

https://codereview.chromium.org/2434063002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_tab_util.h:87: enum UrlType { LastCommitted,
Visible };
On 2016/10/20 00:48:42, nasko (slow) wrote:
> This along with the methods taking it can potentially move into the anonymous
> namespace in the .cc file. I did hit some linker errors when I tried that, but
> didn't dig further.

I'm fine with it here (and think it should be; see below).  But we should use an
enum class, and unfortunately chrome requires SCREAMING_STYLE enums (even though
it hurts my ears).

https://codereview.chromium.org/2434063002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_tab_util.h:107: static
std::unique_ptr<api::tabs::Tab> CreateTabObjectVisibleUrl(
In the tabs API, we'll also scrub the tab object of private data (like urls and
favicons) if the extension doesn't have the "tabs" permission. As such, I think
in this context, "VisibleUrl" is a little ambiguous (visible vs last committed,
or visible meaning present?).  Overall, I'd probably just recommend using the
overload that takes the UrlType in the places we want to specify using the
visible url, rather than adding this separate method.