Blacklist renaming inside blink::protocol namespace.

Blacklist renaming inside blink::protocol namespace.

BUG=680343
Review-Url: https://codereview.chromium.org/2625903006
Cr-Commit-Position: refs/heads/master@{#443432}
Committed: https://chromium.googlesource.com/chromium/src/+/bc607cdd4b82a2f67864b3a33d5368d595b03a96

[Łukasz Anforowicz, 2017-01-12 01:08:36]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org, nasko@chromium.org

[Łukasz Anforowicz, 2017-01-12 01:08:36]

nasko@, can you confirm (and maybe add a specific example to
https://crbug.com/680343) that we need to stop renaming stuff under
blink::protocol namespace?

dcheng@, (assuming desirability of blacklisting) would the changes here look
okay?  In theory we also should tweak |decl_has_qualifier_to_blink_namespace|,
but I have trouble making it work, so I hope this CL is Good Enough (tm).

[dcheng, 2017-01-12 10:27:09]

LGTM assuming this fixes things.

(CCing danakj@, who hit some of these issues with one implementation overriding
both Blink and non-Blink functions last time we tried this)

[danakj, 2017-01-12 17:04:12]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2017-01-12 17:04:12]

This isn't using includeAllOverriddenMethods so I'm having trouble telling
if/how this would assert if we end up with problems like dcheng mentions?

[Łukasz Anforowicz, 2017-01-12 17:08:51]

On 2017/01/12 17:04:12, danakj (slow) wrote:
> This isn't using includeAllOverriddenMethods so I'm having trouble telling
> if/how this would assert if we end up with problems like dcheng mentions?

FWIW, I haven't seen any "conflicting overrides" asserts from
rewrite_to_chrome_style, when running the tool with blacklisting of
blink::protocol namespace.

OTOH, I am not sure if this is what dcheng@ is mentioning - maybe there are
other cases where a method overrides both renamed and non-renamed base method
(if yes, then it is undesirable and a bit surprising that the tool doesn't
assert about these other cases;  OTOH, I think one of nasko@'s findings about
didStopLoading [or didFailLoading?] was of that nature).

[danakj, 2017-01-12 18:48:36]

On Thu, Jan 12, 2017 at 12:08 PM, <lukasza@chromium.org> wrote:

> On 2017/01/12 17:04:12, danakj (slow) wrote:
> > This isn't using includeAllOverriddenMethods so I'm having trouble
> telling
> > if/how this would assert if we end up with problems like dcheng mentions?
>
> FWIW, I haven't seen any "conflicting overrides" asserts from
> rewrite_to_chrome_style, when running the tool with blacklisting of
> blink::protocol namespace.
>

I'm wondering if it's even checking tho, or only checking blink:: vs not
blink::. I don't see how the assert would be reached from the new
blink::protocol code.


>
> OTOH, I am not sure if this is what dcheng@ is mentioning - maybe there
> are
> other cases where a method overrides both renamed and non-renamed base
> method
> (if yes, then it is undesirable and a bit surprising that the tool doesn't
> assert about these other cases; OTOH, I think one of nasko@'s findings
> about
> didStopLoading [or didFailLoading?] was of that nature).
>
> https://codereview.chromium.org/2625903006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Łukasz Anforowicz, 2017-01-12 23:57:29]

Rationale for blacklisting blink::protocol namespace is in
https://crbug.com/680343#c1.  I am going to push this CL out to CQ.

[Łukasz Anforowicz, 2017-01-12 23:57:32]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2017-01-12 23:58:31]

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, 2017-01-13 01:25:01]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484265452969330, "parent_rev":
"64fa7e77502cc896a5ba7d8eab1dd19447fda467", "commit_rev":
"bc607cdd4b82a2f67864b3a33d5368d595b03a96"}

[commit-bot: I haz the power, 2017-01-13 01:25:31]

Description was changed from

==========
Blacklist renaming inside blink::protocol namespace.

BUG=680343
==========

to

==========
Blacklist renaming inside blink::protocol namespace.

BUG=680343

Review-Url: https://codereview.chromium.org/2625903006
Cr-Commit-Position: refs/heads/master@{#443432}
Committed:
https://chromium.googlesource.com/chromium/src/+/bc607cdd4b82a2f67864b3a33d53...
==========

[commit-bot: I haz the power, 2017-01-13 01:25:32]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/bc607cdd4b82a2f67864b3a33d53...

[danakj, 2017-01-13 16:32:22]

OK good I confirmed that this does notice if blink and blink::protocol virtuals
collide with an override.

ERROR:
/usr/local/google/home/danakj/s/c/src/tools/clang/rewrite_to_chrome_style/tests/namespaces-actual.cc:30:3:
blink::ChildOfBoth::myVirtual method overrides some virtual methods that will be
automatically renamed and some that won't be renamed.
Overriden method that will be renamed:
/usr/local/google/home/danakj/s/c/src/tools/clang/rewrite_to_chrome_style/tests/namespaces-actual.cc:16:3:
blink::ParentInBlink::myVirtual
Overriden method that will not be renamed:
/usr/local/google/home/danakj/s/c/src/tools/clang/rewrite_to_chrome_style/tests/namespaces-actual.cc:24:3:
blink::protocol::ParentInBlinkProtocol::myVirtual