Upstream iOS web code.

Upstream iOS web code.

This CL upstreams part of the iOS web code.
This code is needed in order to upstream the Translate component.

Committed: https://crrev.com/8eabfbc0434d494dbb9f45108bb9c73757e90194
Cr-Commit-Position: refs/heads/master@{#307463}

[droger, 2014-12-04 13:08:02]

droger@chromium.org changed reviewers:
+ stuartmorgan@chromium.org

[droger, 2014-12-04 13:08:03]

Note: this CL depends on https://codereview.chromium.org/777183002/

[stuartmorgan, 2014-12-04 23:22:58]

LGTM with some cleanup.

https://codereview.chromium.org/780873002/diff/1/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/780873002/diff/1/ios/web/ios_web.gyp#newcode25
ios/web/ios_web.gyp:25: 'load_committed_details.h',
This is wrong; the correct path is the only added below.

https://codereview.chromium.org/780873002/diff/1/ios/web/public/string_util.h
File ios/web/public/string_util.h (right):

https://codereview.chromium.org/780873002/diff/1/ios/web/public/string_util.h...
ios/web/public/string_util.h:17: base::string16
GetStringByClippingLastWord(const base::string16& contents,
We should revisit this at some point to figure out if we can put it somewhere
better than web; it's pretty random a part of web/public/

https://codereview.chromium.org/780873002/diff/1/ios/web/public/web_state/web...
File ios/web/public/web_state/web_state_observer.h (right):

https://codereview.chromium.org/780873002/diff/1/ios/web/public/web_state/web...
ios/web/public/web_state/web_state_observer.h:18: class WebStateObserver {
Whoops :( Could you add a class-level comment while you're here?

https://codereview.chromium.org/780873002/diff/1/ios/web/public/web_state/web...
ios/web/public/web_state/web_state_observer.h:59: friend class WebStateImpl;
Shouldn't there be a forward declaration for this class?

https://codereview.chromium.org/780873002/diff/1/ios/web/string_util_unittest.cc
File ios/web/string_util_unittest.cc (right):

https://codereview.chromium.org/780873002/diff/1/ios/web/string_util_unittest...
ios/web/string_util_unittest.cc:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
This test file should be added to ios/ios_tests_unit.gyp

(It's really web unit tests, and should be moved/renamed accordingly, but nobody
has gotten around to it. For now we can keep adding tests to it though.)

https://codereview.chromium.org/780873002/diff/1/ios/web/web_state/web_state_...
File ios/web/web_state/web_state_observer.cc (right):

https://codereview.chromium.org/780873002/diff/1/ios/web/web_state/web_state_...
ios/web/web_state/web_state_observer.cc:39: web_state_ = NULL;
Could you change this to nullptr?

[droger, 2014-12-09 13:14:43]

Thanks for the review.

https://codereview.chromium.org/780873002/diff/1/ios/web/public/string_util.h
File ios/web/public/string_util.h (right):

https://codereview.chromium.org/780873002/diff/1/ios/web/public/string_util.h...
ios/web/public/string_util.h:17: base::string16
GetStringByClippingLastWord(const base::string16& contents,
On 2014/12/04 23:22:58, stuartmorgan wrote:
> We should revisit this at some point to figure out if we can put it somewhere
> better than web; it's pretty random a part of web/public/

I agree.
This function has only one caller, and it is in translate.
It was originally in translate, but Shreyas moved it to chrome/common here:
https://chromereviews.googleplex.com/80307013/

I tried to move it back to translate, but apparently there was a discussion it
was decided to put it in web. See comments here:
https://chromereviews.googleplex.com/96607013/diff/180001/components/translat...

[droger, 2014-12-09 13:36:50]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-12-09 13:37:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/780873002/60001

[commit-bot: I haz the power, 2014-12-09 13:57:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-09 13:58:19]

Patchset 4 (id:??) landed as
https://crrev.com/8eabfbc0434d494dbb9f45108bb9c73757e90194
Cr-Commit-Position: refs/heads/master@{#307463}

[tfarina, 2014-12-12 18:41:07]

https://codereview.chromium.org/780873002/diff/60001/ios/web/public/web_state...
File ios/web/public/web_state/web_state_observer.h (right):

https://codereview.chromium.org/780873002/diff/60001/ios/web/public/web_state...
ios/web/public/web_state/web_state_observer.h:21: WebState* web_state() const {
return web_state_; }
this is not fully const-correctness. Could you make this:

const WebState* web_state() const { ... }

And if necessary add:

WebState* web_state() { ... }

?

This can save trouble later on.

https://codereview.chromium.org/780873002/diff/60001/ios/web/public/web_state...
ios/web/public/web_state/web_state_observer.h:28: const LoadCommittedDetails&
load_details) {};
no need of ';' in those virtual methods.

[stuartmorgan, 2014-12-12 19:16:45]

https://codereview.chromium.org/780873002/diff/60001/ios/web/public/web_state...
File ios/web/public/web_state/web_state_observer.h (right):

https://codereview.chromium.org/780873002/diff/60001/ios/web/public/web_state...
ios/web/public/web_state/web_state_observer.h:21: WebState* web_state() const {
return web_state_; }
On 2014/12/12 18:41:07, tfarina wrote:
> this is not fully const-correctness. Could you make this:
> 
> const WebState* web_state() const { ... }
> 
> And if necessary add:
> 
> WebState* web_state() { ... }
> 
> ?
> 
> This can save trouble later on.

This matches WebContentsObserver's WebContents getter, by design. I don't want
to diverge without a good reason, so if you want to change this please propose a
patch that changes it for both locations and ensure that it works in all the
code using content/

[stuartmorgan, 2014-12-12 19:19:57]

https://codereview.chromium.org/780873002/diff/60001/ios/web/public/web_state...
File ios/web/public/web_state/web_state_observer.h (right):

https://codereview.chromium.org/780873002/diff/60001/ios/web/public/web_state...
ios/web/public/web_state/web_state_observer.h:28: const LoadCommittedDetails&
load_details) {};
On 2014/12/12 18:41:07, tfarina wrote:
> no need of ';' in those virtual methods.

If you find style nits in already-submitted code, it's generally much more
useful to just send a patch fixing them than to drive-by nit the CL after the
fact.