mandoline: Add back/forward support and UI.

mandoline: Add back/forward support and UI.

BUG=526268
Committed: https://crrev.com/f72ad72272047f3a1471de9f37e0cba7174ac0b0
Cr-Commit-Position: refs/heads/master@{#348673}

[Elliot Glaysher, 2015-09-04 19:45:42]

erg@chromium.org changed reviewers:
+ jam@chromium.org

[Elliot Glaysher, 2015-09-04 19:52:21]

erg@chromium.org changed reviewers:
+ msw@chromium.org
- jam@chromium.org

[Elliot Glaysher, 2015-09-04 19:52:22]

redirect to msw

[msw, 2015-09-04 22:05:01]

Only a couple big comments, lots of minor ones.

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
File components/web_view/navigation_entry.cc (right):

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
components/web_view/navigation_entry.cc:21:
mojo::common::BlockingCopyToString(request->body[i].Pass(), &body_[0]);
It looks like this repeatedly overwrites body_[0]. Do you mean &body_[i]?

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
components/web_view/navigation_entry.cc:37: // WebKit sometimes gives up empty
data to append. These aren't
I don't see any removal of empty data; is this an old comment?

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
components/web_view/navigation_entry.cc:46:
body.push_back(data_pipe.consumer_handle.Pass());
nit: use request->body directly and avoid the local |body|?

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
components/web_view/navigation_entry.cc:49: &num_bytes,
Maybe [d]check that num_bytes == body_data.size() after this?

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
File components/web_view/navigation_entry.h (right):

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
components/web_view/navigation_entry.h:7: 
nit: include <vector> and <string>

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
components/web_view/navigation_entry.h:14: // them off to other processes.
However, moving back and forward requires
Do you mean "moving back and then forward" (ie. to [bar]->[foo]->[bar], so [bar]
was the 'same' url request sent multiple times)? Perhaps clarify the comment a
bit, maybe move the last sentence to the top? Overall, consider maybe something
like "This class caches data associated with a mojo::URLRequest and vends copies
of the request for the WebView system. Copies are used for repeated navigations,
but URLRequest structs are not copyable, as they contain handles to transfer
large amounts of data between processes." Maybe describing the object as a
copyable wrapper for URLRequests would help?

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
components/web_view/navigation_entry.h:19: class NavigationEntry {
As we discussed, I'm surprised we don't have a more general mechanism to copy
mojo structs that contain handles... As this is the first instance, maybe set
precedent by renaming this URLRequestCloneable, URLRequestCopyable, or
URLRequestCloner, or something?

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
components/web_view/navigation_entry.h:21: NavigationEntry(mojo::URLRequestPtr
incoming_mojo_request);
optional nit: rename incoming_mojo_request to request or original_request?

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
components/web_view/navigation_entry.h:21: NavigationEntry(mojo::URLRequestPtr
incoming_mojo_request);
Explicit

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
components/web_view/navigation_entry.h:25: mojo::URLRequestPtr AsURLRequest();
nit: rename Clone or Copy?

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/nav...
components/web_view/navigation_entry.h:25: mojo::URLRequestPtr AsURLRequest();
Const function

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/pub...
File components/web_view/public/interfaces/web_view.mojom (right):

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/pub...
components/web_view/public/interfaces/web_view.mojom:17: BackForwardChanged(bool
back_enabled, bool forward_enabled);
nit: Backward in function and arg names to match forward.

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/pub...
components/web_view/public/interfaces/web_view.mojom:30: // Moves forward and
backwards.
nit: match trailing 's' chars (ie. use "forwards" or "backward")

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/pub...
components/web_view/public/interfaces/web_view.mojom:31: GoBack();
nit: "GoBackward" to match "GoForward"

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/pub...
components/web_view/public/interfaces/web_view.mojom:32: GoForward();
q: Would it make sense to have a single function that takes an offset into the
navigation stack? ie. GoBackwardOrForward(int offset); That would allow jumps 2+
navigations backward/forward without iterating here. (like chrome's drop down
backward/forward menu buttons)

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/web...
File components/web_view/web_view_impl.cc (right):

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/web...
components/web_view/web_view_impl.cc:80:
client_->BackForwardChanged(back_list_.size() > 0, forward_list_.size() > 0);
nit: use !empty?

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/web...
components/web_view/web_view_impl.cc:108: if (back_list_.size() == 0)
nit: empty

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/web...
components/web_view/web_view_impl.cc:122: if (forward_list_.size() == 0)
nit: empty

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/web...
File components/web_view/web_view_impl.h (right):

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/web...
components/web_view/web_view_impl.h:100: ScopedVector<NavigationEntry>
back_list_;
nit: |backward_list_| to match |forward_list_|

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/web...
components/web_view/web_view_impl.h:101: ScopedVector<NavigationEntry>
forward_list_;
Consider having a single list and an iterator for the current element.
(that should make going back/forward cleaner; no juggling list entries)

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/web...
components/web_view/web_view_impl.h:103: // The request which is either
currently loading or completed loading. On a
nit: consider "The current request, which may be loading or completed."

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/web...
components/web_view/web_view_impl.h:104: // new navigation where this
Incomplete sentence

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/web...
components/web_view/web_view_impl.h:105: scoped_ptr<NavigationEntry>
current_page_request_;
nit: consider |current_request_|

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
File mandoline/ui/desktop_ui/browser_commands.h (right):

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
mandoline/ui/desktop_ui/browser_commands.h:14: BrowserCommand_GoBack,
nit: Backward to match Forward

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
File mandoline/ui/desktop_ui/browser_window.cc (right):

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
mandoline/ui/desktop_ui/browser_window.cc:37: back_button_(new
views::LabelButton(this, base::ASCIIToUTF16("Back"))),
lol, "Back" string...

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
mandoline/ui/desktop_ui/browser_window.cc:42: SetLayoutManager(layout_);
BoxLayout might be simpler, I think it's got some newish flex functionality so
the omnibox would fill the horizontal space.

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
mandoline/ui/desktop_ui/browser_window.cc:64: void SetBackForwardEnabled(bool
back_enabled, bool forward_enabled) {
nit: "backward" in function and arg name to match forward

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
File mandoline/ui/desktop_ui/browser_window.h (right):

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
mandoline/ui/desktop_ui/browser_window.h:18: #include
"ui/views/controls/button/button.h"
Remove; no longer needed.

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
mandoline/ui/desktop_ui/browser_window.h:29: class LabelButton;
Remove; no longer needed.

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
mandoline/ui/desktop_ui/browser_window.h:86: void GoBack();
nit: Backward to match Forward

[Elliot Glaysher, 2015-09-04 22:54:48]

ptal

As discussed in person, I am ignoring the request to use "backward" since it is
the "back" button and the "forward" button.

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/pub...
File components/web_view/public/interfaces/web_view.mojom (right):

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/pub...
components/web_view/public/interfaces/web_view.mojom:32: GoForward();
On 2015/09/04 22:05:00, msw wrote:
> q: Would it make sense to have a single function that takes an offset into the
> navigation stack? ie. GoBackwardOrForward(int offset); That would allow jumps
2+
> navigations backward/forward without iterating here. (like chrome's drop down
> backward/forward menu buttons)

Perhaps farther down the road. For now, though I'd like to keep it this way
because it's much easier to reason about.

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
File mandoline/ui/desktop_ui/browser_window.cc (right):

https://codereview.chromium.org/1326443006/diff/40001/mandoline/ui/desktop_ui...
mandoline/ui/desktop_ui/browser_window.cc:42: SetLayoutManager(layout_);
On 2015/09/04 22:05:00, msw wrote:
> BoxLayout might be simpler, I think it's got some newish flex functionality so
> the omnibox would fill the horizontal space.

Switched to BoxLayout.

[Ben Goodger (Google), 2015-09-04 23:42:22]

ben@chromium.org changed reviewers:
+ ben@chromium.org, sky@chromium.org

[Ben Goodger (Google), 2015-09-04 23:42:22]

+sky also, who is familiar with the content loading flow in webview.

[Ben Goodger (Google), 2015-09-04 23:44:36]

On 2015/09/04 23:42:22, Ben Goodger (Google) wrote:
> +sky also, who is familiar with the content loading flow in webview.

btw it's fine to add this ui.

note though that this back forward support is very primitive. i.e. it doesn't
deal with subframe navigations. e.g. if I navigate to a page that has a subframe
and then navigate the subframe, the subframe navigation should surface in the
back forward list. if I navigate the top level frame the subframe should be
navigated to the place it was last navigated to, not its initial state if i were
to load the top level frame from scratch. etc. etc.

[msw, 2015-09-04 23:49:00]

lgtm with nits, modulo feedback from Ben and Scott.

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/pub...
File components/web_view/public/interfaces/web_view.mojom (right):

https://codereview.chromium.org/1326443006/diff/40001/components/web_view/pub...
components/web_view/public/interfaces/web_view.mojom:32: GoForward();
On 2015/09/04 22:54:48, Elliot Glaysher wrote:
> On 2015/09/04 22:05:00, msw wrote:
> > q: Would it make sense to have a single function that takes an offset into
the
> > navigation stack? ie. GoBackwardOrForward(int offset); That would allow
jumps
> 2+
> > navigations backward/forward without iterating here. (like chrome's drop
down
> > backward/forward menu buttons)
> 
> Perhaps farther down the road. For now, though I'd like to keep it this way
> because it's much easier to reason about.

Eh, I'd argue the opposite, but not strongly, so I'll acquiesce.

https://codereview.chromium.org/1326443006/diff/60001/components/web_view/web...
File components/web_view/web_view_impl.h (right):

https://codereview.chromium.org/1326443006/diff/60001/components/web_view/web...
components/web_view/web_view_impl.h:28: class URLRequestCloneable;
nit: abc order

https://codereview.chromium.org/1326443006/diff/60001/mandoline/ui/desktop_ui...
File mandoline/ui/desktop_ui/browser_window.cc (right):

https://codereview.chromium.org/1326443006/diff/60001/mandoline/ui/desktop_ui...
mandoline/ui/desktop_ui/browser_window.cc:29: using views::BoxLayout;
nit: remove, inline one more "views::".

[sky, 2015-09-08 16:00:33]

https://codereview.chromium.org/1326443006/diff/60001/components/web_view/pub...
File components/web_view/public/interfaces/web_view.mojom (right):

https://codereview.chromium.org/1326443006/diff/60001/components/web_view/pub...
components/web_view/public/interfaces/web_view.mojom:17: BackForwardChanged(bool
back_enabled, bool forward_enabled);
How about enums instead of bools?

https://codereview.chromium.org/1326443006/diff/60001/components/web_view/url...
File components/web_view/url_request_cloneable.cc (right):

https://codereview.chromium.org/1326443006/diff/60001/components/web_view/url...
components/web_view/url_request_cloneable.cc:19:
body_(original_request->body.size()) {
I believe this force body_ to non-null, even if original_request->body is null.
I suspect we don't want that.

https://codereview.chromium.org/1326443006/diff/60001/components/web_view/web...
File components/web_view/web_view_impl.cc (right):

https://codereview.chromium.org/1326443006/diff/60001/components/web_view/web...
components/web_view/web_view_impl.cc:118:
client_->TopLevelNavigate(new_request.Pass());
TopLevelNavigate is intended for requests from the page to navigate the topmost
frame. It doesn't make sense that a request from the client to GoBack() results
in calling back to the client to TopLevelNavigate(). GoBack/GoForward should
call through to LocalRequest.

https://codereview.chromium.org/1326443006/diff/60001/components/web_view/web...
File components/web_view/web_view_impl.h (right):

https://codereview.chromium.org/1326443006/diff/60001/components/web_view/web...
components/web_view/web_view_impl.h:98: // It would be nice if mojo::URLRequest
were cloneable; however, its use of
As Ben said, this needs to tie into frame navigations.

[Elliot Glaysher, 2015-09-09 19:19:02]

ptal

Do you want the subframe stuff to block this patch? That seems like it handles
the 99% case, where framset/iframe navigation seems to be the rare case.

[sky, 2015-09-09 20:17:27]

https://codereview.chromium.org/1326443006/diff/100001/components/web_view/ur...
File components/web_view/url_request_cloneable.cc (right):

https://codereview.chromium.org/1326443006/diff/100001/components/web_view/ur...
components/web_view/url_request_cloneable.cc:23:
mojo::common::BlockingCopyToString(original_request->body[i].Pass(),
I'm more worried about the effects of this. I think you should separate this
class into its own patch and move the loading onto a worker thread with Clone()
serving from that.

[sky, 2015-09-09 20:43:28]

https://codereview.chromium.org/1326443006/diff/100001/components/web_view/ur...
File components/web_view/url_request_cloneable.cc (right):

https://codereview.chromium.org/1326443006/diff/100001/components/web_view/ur...
components/web_view/url_request_cloneable.cc:23:
mojo::common::BlockingCopyToString(original_request->body[i].Pass(),
On 2015/09/09 20:17:27, sky wrote:
> I'm more worried about the effects of this. I think you should separate this
> class into its own patch and move the loading onto a worker thread with
Clone()
> serving from that.

Elliot pointed out that body isn't the content, but the request body. So, this
should be fine for now.

https://codereview.chromium.org/1326443006/diff/100001/components/web_view/we...
File components/web_view/web_view_impl.cc (right):

https://codereview.chromium.org/1326443006/diff/100001/components/web_view/we...
components/web_view/web_view_impl.cc:105:
pending_load_->Init(current_page_request_->Clone());
Don't you need to prune the forward_list_ at some point? Say you go back two
pages, then type a new url, the two requests that were on the front should be
removed.

https://codereview.chromium.org/1326443006/diff/100001/mandoline/ui/desktop_u...
File mandoline/ui/desktop_ui/browser_window.cc (right):

https://codereview.chromium.org/1326443006/diff/100001/mandoline/ui/desktop_u...
mandoline/ui/desktop_ui/browser_window.cc:29: class BrowserWindow::ToolbarView :
public views::View,
Move this into its own file. No doubt it's going to get more logic.

https://codereview.chromium.org/1326443006/diff/100001/mandoline/ui/desktop_u...
mandoline/ui/desktop_ui/browser_window.cc:70: NOTIMPLEMENTED();
Shouldn't this be a NOTREACHED?

[sky, 2015-09-09 20:44:00]

Also, you should add test coverage.

[Elliot Glaysher, 2015-09-11 19:07:53]

ptal. I've added tests.

[msw, 2015-09-11 20:07:54]

fwiw, lgtm with nits

https://codereview.chromium.org/1326443006/diff/120001/components/web_view/pu...
File components/web_view/public/interfaces/web_view.mojom (right):

https://codereview.chromium.org/1326443006/diff/120001/components/web_view/pu...
components/web_view/public/interfaces/web_view.mojom:12: DISABLED
nit: trailing comma

https://codereview.chromium.org/1326443006/diff/120001/components/web_view/we...
File components/web_view/web_view_apptest.cc (right):

https://codereview.chromium.org/1326443006/diff/120001/components/web_view/we...
components/web_view/web_view_apptest.cc:55: data_file =
data_file.AppendASCII("components")
optional nit: use NormalizePathSeparators for a shortened path construction:
  data_file = data_file.AppendASCII("components/test/data/web_view");
  data_file = data_file.AppendASCII(file).NormalizePathSeparators();

https://codereview.chromium.org/1326443006/diff/120001/components/web_view/we...
components/web_view/web_view_apptest.cc:130: // Our title should have been set
on the final.
nit: "final ..."? "final navigation"?

https://codereview.chromium.org/1326443006/diff/120001/components/web_view/we...
components/web_view/web_view_apptest.cc:163:
EXPECT_EQ(mojom::ButtonState::BUTTON_STATE_ENABLED, last_back_button_state());
nit: EXPECT_EQ(kTestTwoTitle, last_title()); for consistency/thoroughness

https://codereview.chromium.org/1326443006/diff/120001/components/web_view/we...
components/web_view/web_view_apptest.cc:183: // Now make a new navigation to a
third file. This should clear the forward
nit: s/make a new navigation/navigate/ for a one-liner

https://codereview.chromium.org/1326443006/diff/120001/components/web_view/we...
File components/web_view/web_view_impl.cc (right):

https://codereview.chromium.org/1326443006/diff/120001/components/web_view/we...
components/web_view/web_view_impl.cc:96: // When we are performing a top level
load request, we must clear the forward
nit: consider "Clear the forward list when performing a top level load request."

https://codereview.chromium.org/1326443006/diff/120001/components/web_view/we...
File components/web_view/web_view_impl.h (right):

https://codereview.chromium.org/1326443006/diff/120001/components/web_view/we...
components/web_view/web_view_impl.h:48: // Actually performs the load, and tells
our delegate what the state of the
nit: no comma and "tells our delegate the state of the forward/back list".

[sky, 2015-09-11 20:22:51]

LGTM

https://codereview.chromium.org/1326443006/diff/140001/components/web_view/we...
File components/web_view/web_view_apptest.cc (right):

https://codereview.chromium.org/1326443006/diff/140001/components/web_view/we...
components/web_view/web_view_apptest.cc:54:
ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &data_file));
As you have asserts in here you'll want to wrap calls to it with
ASSERT_NO_FATAL_FAILURE.

[Elliot Glaysher, 2015-09-11 20:55:27]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-09-11 20:55:28]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1326443006/#ps160001
(title: "nits and such.")

[commit-bot: I haz the power, 2015-09-11 20:56:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1326443006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1326443006/160001

[commit-bot: I haz the power, 2015-09-11 21:48:26]

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

[commit-bot: I haz the power, 2015-09-11 21:48:27]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2015-09-11 21:49:59]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-09-11 21:50:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1326443006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1326443006/160001

[commit-bot: I haz the power, 2015-09-11 22:34:33]

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

[commit-bot: I haz the power, 2015-09-11 22:34:34]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2015-09-11 22:56:20]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-09-11 22:56:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1326443006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1326443006/160001

[commit-bot: I haz the power, 2015-09-11 23:13:18]

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

[commit-bot: I haz the power, 2015-09-11 23:13:19]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Elliot Glaysher, 2015-09-11 23:31:02]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-09-11 23:31:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1326443006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1326443006/160001

[commit-bot: I haz the power, 2015-09-12 00:05:33]

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

[commit-bot: I haz the power, 2015-09-12 00:05:34]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2015-09-12 00:13:20]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-09-12 00:13:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1326443006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1326443006/160001

[commit-bot: I haz the power, 2015-09-12 03:16:18]

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

[commit-bot: I haz the power, 2015-09-12 03:16:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2015-09-14 17:14:09]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-09-14 17:14:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1326443006/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1326443006/160001

[commit-bot: I haz the power, 2015-09-14 19:02:18]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-09-14 19:02:52]

Patchset 9 (id:??) landed as
https://crrev.com/f72ad72272047f3a1471de9f37e0cba7174ac0b0
Cr-Commit-Position: refs/heads/master@{#348673}

[Devlin, 2015-09-14 20:37:34]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1343773003/ by rdevlin.cronin@chromium.org.

The reason for reverting is: Broke Win x64 GN:
FAILED: ninja -t msvc -e environment.x64 -- E:\b\build\goma/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64/cl.exe" /nologo
/showIncludes /FC @obj/components/web_view/lib/url_request_cloneable.obj.rsp /c
../../components/web_view/url_request_cloneable.cc
/Foobj/components/web_view/lib/url_request_cloneable.obj
/Fdobj/components/web_view/lib_cc.pdb
e:\b\build\slave\win_x64_gn\build\src\components\web_view\url_request_cloneable.cc(43)
: error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win_x64_gn\build\src\components\web_view\url_request_cloneable.cc(43)
: warning C4267: 'initializing' : conversion from 'size_t' to 'uint32_t',
possible loss of data
ninja: build stopped: subcommand failed..

[commit-bot: I haz the power, 2015-09-23 12:35:14]

Patchset 9 (id:??) landed as
https://crrev.com/f72ad72272047f3a1471de9f37e0cba7174ac0b0
Cr-Commit-Position: refs/heads/master@{#348673}