Issue 2902403002: Add KVO compliant title property to CWVWebView.

michaeldo

michaeldo@chromium.org changed reviewers: + eugenebut@chromium.org

3 years, 7 months ago (2017-05-25 17:13:30 UTC) #1

Eugene But (OOO till 7-30)

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_web_view_kvo_inttest.mm File ios/web_view/test/chrome_web_view_kvo_inttest.mm (right): https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_web_view_kvo_inttest.mm#newcode12 ios/web_view/test/chrome_web_view_kvo_inttest.mm:12: #import "ios/web_view/test/observers/string_observer.h" Do we actually need 2 different observers ...

3 years, 7 months ago (2017-05-25 17:43:03 UTC) #3

michaeldo

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_web_view_kvo_inttest.mm File ios/web_view/test/chrome_web_view_kvo_inttest.mm (right): https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_web_view_kvo_inttest.mm#newcode12 ios/web_view/test/chrome_web_view_kvo_inttest.mm:12: #import "ios/web_view/test/observers/string_observer.h" On 2017/05/25 17:43:03, Eugene But wrote: > ...

3 years, 7 months ago (2017-05-26 14:59:02 UTC) #4

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_we...
File ios/web_view/test/chrome_web_view_kvo_inttest.mm (right):

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_we...
ios/web_view/test/chrome_web_view_kvo_inttest.mm:12: #import
"ios/web_view/test/observers/string_observer.h"
On 2017/05/25 17:43:03, Eugene But wrote:
> Do we actually need 2 different observers for BOOL and NSString? From KVO
> perspective both are just id. Also implementation of subclasses differs only
by
> type, which seems to be redundant for dynamically types language.

It's true that they are not required as the one that uses id would work fine.
However there are lots of places that access the value in these tests and I
think it is better to have the specific classes for common use case types (like
here with strings and bool). I don't think we would want subclasses for every
class type, but I personally think it's clearer with the specific classes in
these cases.

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_we...
ios/web_view/test/chrome_web_view_kvo_inttest.mm:109: "<a id='link_1' href='" +
page_2_url.spec() + "'>Link 1</a>";
On 2017/05/25 17:43:03, Eugene But wrote:
> Please se StringPrint instead of +

Done.

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_we...
ios/web_view/test/chrome_web_view_kvo_inttest.mm:119:
EXPECT_NSEQ(observer.lastValue, page_2_title);
On 2017/05/25 17:43:03, Eugene But wrote:
> EXPECT_NSEQ takes first argument as expected value and second as actual

Done.

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_we...
File ios/web_view/test/chrome_web_view_test.mm (right):

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_we...
ios/web_view/test/chrome_web_view_test.mm:35: std::string html =
"<html><head><title>" + title + "</title></head><body>" +
On 2017/05/25 17:43:03, Eugene But wrote:
> Please use StringPrint instead of +

Done.

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_we...
ios/web_view/test/chrome_web_view_test.mm:48: std::string title = std::string();
On 2017/05/25 17:43:03, Eugene But wrote:
> s/std::string title = std::string();/std::string title;

Done.

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_we...
ios/web_view/test/chrome_web_view_test.mm:48: std::string title = std::string();
On 2017/05/25 17:43:03, Eugene But wrote:
> It's quite hard to tell what this method does. Do you want to refactor in into
> smaller well named functions and add comments?

I agree, this method went through many local revisions and is a bit ugly.

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_we...
ios/web_view/test/chrome_web_view_test.mm:48: std::string title = std::string();
On 2017/05/25 17:43:03, Eugene But wrote:
> It's quite hard to tell what this method does. Do you want to refactor in into
> smaller well named functions and add comments?

I agree, this method went through many local revisions and is a bit ugly.

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_we...
ios/web_view/test/chrome_web_view_test.mm:107: std::string url_params =
std::string();
On 2017/05/25 17:43:03, Eugene But wrote:
> Please describe in comments how query is used.

Done.

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_we...
ios/web_view/test/chrome_web_view_test.mm:110: url_params +=
kPageHTMLTitleParamName + base64_title;
On 2017/05/25 17:43:03, Eugene But wrote:
> Please use GURL API to construct the url.

Done.

Eugene But (OOO till 7-30)

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_web_view_kvo_inttest.mm File ios/web_view/test/chrome_web_view_kvo_inttest.mm (right): https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_web_view_kvo_inttest.mm#newcode12 ios/web_view/test/chrome_web_view_kvo_inttest.mm:12: #import "ios/web_view/test/observers/string_observer.h" On 2017/05/26 14:59:01, michaeldo wrote: > On ...

3 years, 6 months ago (2017-05-31 22:10:01 UTC) #6

michaeldo

https://codereview.chromium.org/2902403002/diff/20001/ios/web_view/test/chrome_web_view_test.h File ios/web_view/test/chrome_web_view_test.h (right): https://codereview.chromium.org/2902403002/diff/20001/ios/web_view/test/chrome_web_view_test.h#newcode37 ios/web_view/test/chrome_web_view_test.h:37: GURL GetUrlForPageWithHTMLBody(const std::string& html); On 2017/05/31 22:10:01, Eugene But ...

3 years, 6 months ago (2017-06-01 15:45:54 UTC) #7

Eugene But (OOO till 7-30)

https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_web_view_kvo_inttest.mm File ios/web_view/test/chrome_web_view_kvo_inttest.mm (right): https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_web_view_kvo_inttest.mm#newcode12 ios/web_view/test/chrome_web_view_kvo_inttest.mm:12: #import "ios/web_view/test/observers/string_observer.h" On 2017/05/31 22:10:00, Eugene But wrote: > ...

3 years, 6 months ago (2017-06-01 15:53:54 UTC) #8

michaeldo

On 2017/06/01 15:53:54, Eugene But wrote: > https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_web_view_kvo_inttest.mm > File ios/web_view/test/chrome_web_view_kvo_inttest.mm (right): > > https://codereview.chromium.org/2902403002/diff/1/ios/web_view/test/chrome_web_view_kvo_inttest.mm#newcode12 ...

3 years, 6 months ago (2017-06-01 18:41:29 UTC) #9

michaeldo

The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the ...

3 years, 6 months ago (2017-06-01 20:05:57 UTC) #12

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2902403002/80001

3 years, 6 months ago (2017-06-01 20:06:49 UTC) #13

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496347556704970, "parent_rev": "ee1c707a83bef0504aaabd120d44fbcef56f7380", "commit_rev": "2011b27fb8555b25ecc46b651b022b54a85d54a9"}

3 years, 6 months ago (2017-06-01 21:43:47 UTC) #14

commit-bot: I haz the power

Description was changed from ========== Add KVO compliant title property to CWVWebView. The GetUrlForPagewithTitle method ...

3 years, 6 months ago (2017-06-01 21:43:58 UTC) #15

commit-bot: I haz the power

3 years, 6 months ago (2017-06-01 21:43:59 UTC) #16

Message was sent while issue was closed.

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2011b27fb8555b25ecc46b651b02...

Issue 2902403002: Add KVO compliant title property to CWVWebView. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Respond to comments. #

Patch Set 3 : Cleanup. Respond to comments. #

Patch Set 4 : Remove type specific observers. #

Patch Set 5 : Remove observers directory. #

Messages