Add 'get' prefix for Settings.in generated code.

Add 'get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/25eb1046e8317b6cf33fb0d831857e80c29cffcc
Cr-Commit-Position: refs/heads/master@{#441547}

[nasko, 2016-12-20 20:05:17]

Description was changed from

==========
Add 'get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

BUG=582312
==========

to

==========
Add 'get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[nasko, 2016-12-20 20:05:18]

Description was changed from

==========
Add 'get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add 'get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[nasko, 2016-12-20 20:07:06]

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

[commit-bot: I haz the power, 2016-12-20 20:07:35]

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

[nasko, 2016-12-20 21:08:52]

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

[nasko, 2016-12-20 21:08:53]

Hey Daniel and Lukasz,
Can one of you review this CL to pre-land the Settings.in getter fix?
Thanks in advance!
Nasko

[Łukasz Anforowicz, 2016-12-21 21:07:36]

Non-OWNER LGTM, but:

1) please double-check the choice to use |upper_first| VS |upper_first_letter|
(see my comments below).

and

2) I wonder if we will eventually need to also tackle "set" -> "Set" (and
whether doing this in this CL might make sense).

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/templates/InternalSettingsGenerated.cpp.tmpl
(right):

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/InternalSettingsGenerated.cpp.tmpl:14:
, m_{{setting.name}}(page->settings().Get{{setting.name|upper_first}}())
|upper_first| causes ...FEtch... issue.  I wonder if we should switch to
|upper_first_letter| or tweak implementation of |upper_first| so that it checks
what is *after* the acronym:

# not tested / pseudocode / I don't know if len(...) and is_upper_case are a
thing in Python:
def upper_first(name):
    """Return name with first letter or initial acronym uppercased."""
    for acronym in ACRONYMS:
-        if name.startswith(acronym.lower()):
+        if name.startswith(acronym.lower()) and len(name) > len(acronym) and
is_upper_case(name[len(acronym)]):
            return name.replace(acronym.lower(), acronym, 1)
    return upper_first_letter(name)

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSImageSetValue.cpp (right):

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSImageSetValue.cpp:120: if
(document.settings() && document.settings()->GetFEtchImagePlaceholders())
I think I understand why we get ...FEtch... here - upper_first tries to
upper-case acronyms at the beginning of an identifier and "FE" is one of known
acronyms.

So - I think we should consistently switch away from |upper_first| everywhere:
- To |upper_first_letter| I think?
- And we also need to pass |upper_first_letter| to Jinja via
build/scripts/make_internal_settings.py and build/scripts/make_settings.py -
something similar to:

    filters = {
        'upper_first': name_utilities.upper_first,
--->    'upper_first_letter': name_utilities.upper_first_letter,   <----

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right):

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1495:
settings->GetDOMPasteAllowed();
AFAICT, upper_first_letter would still work here (luckily the "DOM" acronym was
already uppercase in the "before" state of the code).

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right):

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:255: :
m_policy(preloader->document()->settings()->GetCSSExternalScannerPreload()
This is an interesting case for choosing between |upper_first| VS
|upper_first_letter|.

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/LinkLoader.cpp:143: if (settings &&
settings->GetDNSPrefetchingEnabled() && href.isValid() &&
Another interesting case for |upper_first| VS |upper_first_letter|.

[dcheng, 2016-12-22 07:09:18]

Do we want to rename Set in this CL as well, for (local) consistency?

[dcheng, 2016-12-22 07:10:09]

Also, I would have thought that the renamer would skip all of these, since the
identifiers live in generated code. Any clue why the matcher that excludes decls
from generated files doesn't match against this?

[Łukasz Anforowicz, 2016-12-28 00:14:25]

On 2016/12/22 07:09:18, dcheng wrote:
> Do we want to rename Set in this CL as well, for (local) consistency?

No opinion.


On 2016/12/22 07:10:09, dcheng wrote:
> Also, I would have thought that the renamer would skip all of these, since the
> identifiers live in generated code. Any clue why the matcher that excludes
decls
> from generated files doesn't match against this?

Here is the situation: the generated code (definition of the
SETTINGS_GETTERS_AND_SETTERS macro in gen/blink/core/SettingsMacros.h) is
included from non-generated code (from
third_party/WebKit/Source/core/frame/Settings.h) and the non-generated code
expands/calls the macro from generated code.

The problem is that the isExpansionInFileMatching matcher looks at a filename of
the following location:
  auto ExpansionLoc = SourceManager.getExpansionLoc(Node.getLocStart());
which points at third_party/WebKit/Source/core/frame/Settings.h.

In theory we could define out own matcher (isDeclInGeneratedFile) that uses
getSpellingLoc instead (this properly points at
gen/blink/core/SettingsMacros.h).


If we can avoid the problem by avoiding the rewrite (i.e. via custom
isDeclInGeneratedFile matcher), then this CL should be probably put on hold
(unless we later decide that we *do* want to rewrite all generated code).  I'll
keep you posted.

[nasko, 2017-01-04 18:18:17]

Description was changed from

==========
Add 'get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add 'Get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

The CL also capitalizes the Set prefix to match.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[nasko, 2017-01-04 18:18:52]

This CL now capitalizes the Set methods too. Please take another look when you
have a chance.

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/templates/InternalSettingsGenerated.cpp.tmpl
(right):

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/InternalSettingsGenerated.cpp.tmpl:14:
, m_{{setting.name}}(page->settings().Get{{setting.name|upper_first}}())
On 2016/12/21 21:07:36, Łukasz Anforowicz wrote:
> |upper_first| causes ...FEtch... issue.  I wonder if we should switch to
> |upper_first_letter| or tweak implementation of |upper_first| so that it
checks
> what is *after* the acronym:
> 
> # not tested / pseudocode / I don't know if len(...) and is_upper_case are a
> thing in Python:
> def upper_first(name):
>     """Return name with first letter or initial acronym uppercased."""
>     for acronym in ACRONYMS:
> -        if name.startswith(acronym.lower()):
> +        if name.startswith(acronym.lower()) and len(name) > len(acronym) and
> is_upper_case(name[len(acronym)]):
>             return name.replace(acronym.lower(), acronym, 1)
>     return upper_first_letter(name)

Done.

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSImageSetValue.cpp (right):

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSImageSetValue.cpp:120: if
(document.settings() && document.settings()->GetFEtchImagePlaceholders())
On 2016/12/21 21:07:36, Łukasz Anforowicz wrote:
> I think I understand why we get ...FEtch... here - upper_first tries to
> upper-case acronyms at the beginning of an identifier and "FE" is one of known
> acronyms.
> 
> So - I think we should consistently switch away from |upper_first| everywhere:
> - To |upper_first_letter| I think?
> - And we also need to pass |upper_first_letter| to Jinja via
> build/scripts/make_internal_settings.py and build/scripts/make_settings.py -
> something similar to:
> 
>     filters = {
>         'upper_first': name_utilities.upper_first,
> --->    'upper_first_letter': name_utilities.upper_first_letter,   <----
> 

Acknowledged.

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right):

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1495:
settings->GetDOMPasteAllowed();
On 2016/12/21 21:07:36, Łukasz Anforowicz wrote:
> AFAICT, upper_first_letter would still work here (luckily the "DOM" acronym
was
> already uppercase in the "before" state of the code).

Acknowledged.

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right):

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:255: :
m_policy(preloader->document()->settings()->GetCSSExternalScannerPreload()
On 2016/12/21 21:07:36, Łukasz Anforowicz wrote:
> This is an interesting case for choosing between |upper_first| VS
> |upper_first_letter|.

The acronyms style for Chromium is to use Css, not CSS. Using upper_first_letter
matches that, so we should be good here.

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/2589143003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/LinkLoader.cpp:143: if (settings &&
settings->GetDNSPrefetchingEnabled() && href.isValid() &&
On 2016/12/21 21:07:36, Łukasz Anforowicz wrote:
> Another interesting case for |upper_first| VS |upper_first_letter|.

Done.

[nasko, 2017-01-04 18:19:12]

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

[commit-bot: I haz the power, 2017-01-04 18:19:32]

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, 2017-01-04 19:14:49]

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

[commit-bot: I haz the power, 2017-01-04 19:14:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nasko, 2017-01-04 23:02:30]

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

[commit-bot: I haz the power, 2017-01-04 23:03:07]

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

[nasko, 2017-01-04 23:52:57]

Patchset #4 (id:60001) has been deleted

[nasko, 2017-01-04 23:56:23]

Description was changed from

==========
Add 'Get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

The CL also capitalizes the Set prefix to match.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add 'get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[nasko, 2017-01-04 23:58:18]

nasko@chromium.org changed reviewers:
+ haraken@chromium.org

[nasko, 2017-01-04 23:58:18]

And after more discussions and finding that we expose internal settings through
IDL to test code, I'm only going to land "get" prefixes for now.

Adding haraken@ for OWNERS review in third_party/WebKit/Source/bindings and
third_party/WebKit/Source/modules/.

[nasko, 2017-01-04 23:59:29]

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

[commit-bot: I haz the power, 2017-01-04 23:59:48]

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

[haraken, 2017-01-05 00:01:16]

LGTM

[dcheng, 2017-01-05 00:05:57]

lgtm

[nasko, 2017-01-05 00:32:00]

The CQ bit was unchecked by nasko@chromium.org

[nasko, 2017-01-05 00:32:51]

The CQ bit was checked by nasko@chromium.org

[nasko, 2017-01-05 00:32:52]

The patchset sent to the CQ was uploaded after l-g-t-m from lukasza@chromium.org
Link to the patchset: https://codereview.chromium.org/2589143003/#ps80001
(title: "Only get prefix and no capitalization.")

[commit-bot: I haz the power, 2017-01-05 00:33:35]

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-05 01:41:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483576371680910,
"parent_rev": "45d1a4cb9cf2b280a2d2349f0bf74057fae52e37", "commit_rev":
"1e413e5c32df561a04fdce21d509b872005a6514"}

[commit-bot: I haz the power, 2017-01-05 01:42:10]

Description was changed from

==========
Add 'get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add 'get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2589143003
==========

[commit-bot: I haz the power, 2017-01-05 01:42:12]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2017-01-05 01:43:52]

Description was changed from

==========
Add 'get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2589143003
==========

to

==========
Add 'get' prefix for Settings.in generated code.

The change is needed to avoid a naming collision after changing from
Blink to Chromium naming style. Settings.in is used to automatically
generate C++ code and it causes a collision between CamelCased type
name and the CamelCase accessor. Therefore we need to prefix the
accessor methods with a get prefix to avoid collisions.

BUG=582312
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/25eb1046e8317b6cf33fb0d831857e80c29cffcc
Cr-Commit-Position: refs/heads/master@{#441547}
==========

[commit-bot: I haz the power, 2017-01-05 01:43:55]

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