[MD settings] advanced font settings extension link

[MD settings] advanced font settings extension link

This CL changes the advanced font settings setting to
either open the extension for installation or open
the extension options, depending on whether the
extension is currently installed.

BUG=597717, 531786
Committed: https://crrev.com/9aebfe150743842f343a0887972960597c6ab3b2
Cr-Commit-Position: refs/heads/master@{#386740}

[dschuyler, 2016-04-11 20:43:46]

Patchset #1 (id:1) has been deleted

[dschuyler, 2016-04-11 20:43:53]

Patchset #1 (id:20001) has been deleted

[dschuyler, 2016-04-11 20:47:40]

dschuyler@chromium.org changed reviewers:
+ dpapad@chromium.org

[dschuyler, 2016-04-11 20:49:43]

On 2016/04/11 20:47:41, dschuyler wrote:

Unit test to follow in the next CL.

[dpapad, 2016-04-11 21:27:30]

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/font_handler.cc (right):

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/font_handler.cc:66:
NotifyAdvancedFontSettingsAvailability();
The communication between JS and C++ starts to get a bit complicated with this
addition, if I understand this correctly, what happens now is

1) JS calls fetchFontsData() which returns a Promise
2) Step1 causes an 'advanced-font-settings-installed' WebUI event to fire (and
therefore the JS handler of that event to execute).
3) The promise returned from 1 is resolved, which triggers other JS code.

Can we simplify this? It seems that we are mixing up two unrelated calls.
Maybe adding an explicit getFontSettingsAvailability() JS method that returns a
Promise, and have JS call that method in ready() similar to the fetchFontsData
call? Then the 'advanced-font-settings-installed' event can be fired only when
something truly changes (OnExtensionLoaded and OnExtensionUnloaded).

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/font_handler.cc:84: Profile* profile =
Profile::FromWebUI(web_ui());
There is already a profile_ member var.

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/font_handler.cc:100: content::BrowserContext*
/*browser_context*/,
Nit: The "/* foo */" inlined comments here and below don't seem to add any
valuable extra information.

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/font_handler.cc:157:
std::unique_ptr<base::StringValue> advancedExtensionUrl(new base::StringValue(
Is the use of a pointer necessary here? I think it can be done in a simpler way.
See more information at next comment.

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/font_handler.cc:163:
response.Set("extensionUrl", std::move(advancedExtensionUrl));
1) How about using SetString() instead?
2) See example at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
where a GURL is passed to JS without the use of a pointer.

[dschuyler, 2016-04-11 22:53:35]

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/font_handler.cc (right):

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/font_handler.cc:66:
NotifyAdvancedFontSettingsAvailability();
On 2016/04/11 21:27:30, dpapad wrote:
> The communication between JS and C++ starts to get a bit complicated with this
> addition, if I understand this correctly, what happens now is
> 
> 1) JS calls fetchFontsData() which returns a Promise
> 2) Step1 causes an 'advanced-font-settings-installed' WebUI event to fire (and
> therefore the JS handler of that event to execute).
> 3) The promise returned from 1 is resolved, which triggers other JS code.
> 
> Can we simplify this? It seems that we are mixing up two unrelated calls.
> Maybe adding an explicit getFontSettingsAvailability() JS method that returns
a
> Promise, and have JS call that method in ready() similar to the fetchFontsData
> call? Then the 'advanced-font-settings-installed' event can be fired only when
> something truly changes (OnExtensionLoaded and OnExtensionUnloaded).

Done.

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/font_handler.cc:84: Profile* profile =
Profile::FromWebUI(web_ui());
On 2016/04/11 21:27:30, dpapad wrote:
> There is already a profile_ member var.

Done.

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/font_handler.cc:100: content::BrowserContext*
/*browser_context*/,
On 2016/04/11 21:27:30, dpapad wrote:
> Nit: The "/* foo */" inlined comments here and below don't seem to add any
> valuable extra information.

Done.

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/font_handler.cc:157:
std::unique_ptr<base::StringValue> advancedExtensionUrl(new base::StringValue(
On 2016/04/11 21:27:30, dpapad wrote:
> Is the use of a pointer necessary here? I think it can be done in a simpler
way.
> See more information at next comment.

Done.

https://codereview.chromium.org/1877923002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/font_handler.cc:163:
response.Set("extensionUrl", std::move(advancedExtensionUrl));
On 2016/04/11 21:27:30, dpapad wrote:
> 1) How about using SetString() instead?
> 2) See example at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
> where a GURL is passed to JS without the use of a pointer.

Done.

[dpapad, 2016-04-11 23:01:13]

LGTM.

Nit: Typo in CL description
"to either open the extension for installation *or* open ..."

[dschuyler, 2016-04-11 23:02:13]

Description was changed from

==========
[MD settings] advanced font settings extension link

This CL changes the advanced font settings setting to
either open the extension for installation for open
the extension options, depending on whether the
extension is currently installed.

BUG=597717, 531786
==========

to

==========
[MD settings] advanced font settings extension link

This CL changes the advanced font settings setting to
either open the extension for installation or open
the extension options, depending on whether the
extension is currently installed.

BUG=597717, 531786
==========

[dschuyler, 2016-04-12 18:12:30]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-04-12 18:12:53]

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

[commit-bot: I haz the power, 2016-04-12 18:18:56]

Description was changed from

==========
[MD settings] advanced font settings extension link

This CL changes the advanced font settings setting to
either open the extension for installation or open
the extension options, depending on whether the
extension is currently installed.

BUG=597717, 531786
==========

to

==========
[MD settings] advanced font settings extension link

This CL changes the advanced font settings setting to
either open the extension for installation or open
the extension options, depending on whether the
extension is currently installed.

BUG=597717, 531786
==========

[commit-bot: I haz the power, 2016-04-12 18:18:57]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-04-12 18:20:29]

Description was changed from

==========
[MD settings] advanced font settings extension link

This CL changes the advanced font settings setting to
either open the extension for installation or open
the extension options, depending on whether the
extension is currently installed.

BUG=597717, 531786
==========

to

==========
[MD settings] advanced font settings extension link

This CL changes the advanced font settings setting to
either open the extension for installation or open
the extension options, depending on whether the
extension is currently installed.

BUG=597717, 531786

Committed: https://crrev.com/9aebfe150743842f343a0887972960597c6ab3b2
Cr-Commit-Position: refs/heads/master@{#386740}
==========

[commit-bot: I haz the power, 2016-04-12 18:20:30]

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