Show third party wallpaper app name in wallpaper picker

Show third party wallpaper app name in wallpaper picker


If current wallpaper is set by third party app/extension, show its name
in wallpaper picker.


BUG=360695
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278399

[bshe, 2014-06-17 14:53:58]

Hey Rob.

Do you mind to take a look at this CL? Thanks!

Biao

[flackr, 2014-06-17 15:20:34]

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/wallpaper_manager/css/wallpaper_manager.css
(right):

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/css/wallpaper_manager.css:170:
#wallpaper-set-by-message[hidden] + .image-picker {
Instead of hardcoding these sizes, can you use a box flex layout to size the
.image-picker to be the available space?

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:257:
$('wallpaper-set-by-message').hidden = false;
Use the :empty CSS selector to automatically hide this element when empty. Then
setting the text content will implicitly make it visible.

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:258:
$('wallpaper-set-by-message').innerText = loadTimeData.getStringF(
s/innerText/textContent

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:361:
this.hideWallpaperSetByMessage_();
Rather than sprinkling this throughout the code, can you add a function which is
called when the wallpaper is changed? Perhaps onWallpaperChanged_.

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:488:
$('wallpaper-set-by-message').innerText = '';
s/innerText/textContent

[bshe, 2014-06-17 19:01:44]

Thanks for review. PTAL.

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/wallpaper_manager/css/wallpaper_manager.css
(right):

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/css/wallpaper_manager.css:170:
#wallpaper-set-by-message[hidden] + .image-picker {
On 2014/06/17 15:20:34, flackr wrote:
> Instead of hardcoding these sizes, can you use a box flex layout to size the
> .image-picker to be the available space?

Done. I remember having some scrolling issues when set the wallpaper webui grid
to flex box. But it no longer reproduce.

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:257:
$('wallpaper-set-by-message').hidden = false;
On 2014/06/17 15:20:34, flackr wrote:
> Use the :empty CSS selector to automatically hide this element when empty.
Then
> setting the text content will implicitly make it visible.

Done.

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:258:
$('wallpaper-set-by-message').innerText = loadTimeData.getStringF(
On 2014/06/17 15:20:34, flackr wrote:
> s/innerText/textContent

Done.

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:361:
this.hideWallpaperSetByMessage_();
On 2014/06/17 15:20:34, flackr wrote:
> Rather than sprinkling this throughout the code, can you add a function which
is
> called when the wallpaper is changed? Perhaps onWallpaperChanged_.

Unfortunately, there are a few private APIs(setWallpaper, setWallpaperIfExist,
setCustomWallpaper, resetWallpaper) that might change wallpaper. And after each
api call, we dont want to do the same thing. This makes adding a general
onWallpaperChanged_ function not trivial and possibly very complicate code. Do
you mind if I keep it this way?

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:488:
$('wallpaper-set-by-message').innerText = '';
On 2014/06/17 15:20:34, flackr wrote:
> s/innerText/textContent

Done. There is only one line left in this function but it is probably still make
sense to keep this function as the function name is more readable than set
textContext to empty. WDYT?

[flackr, 2014-06-18 00:59:48]

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:361:
this.hideWallpaperSetByMessage_();
On 2014/06/17 19:01:44, bshe wrote:
> On 2014/06/17 15:20:34, flackr wrote:
> > Rather than sprinkling this throughout the code, can you add a function
which
> is
> > called when the wallpaper is changed? Perhaps onWallpaperChanged_.
> 
> Unfortunately, there are a few private APIs(setWallpaper, setWallpaperIfExist,
> setCustomWallpaper, resetWallpaper) that might change wallpaper. And after
each
> api call, we dont want to do the same thing. This makes adding a general
> onWallpaperChanged_ function not trivial and possibly very complicate code. Do
> you mind if I keep it this way?

Even if the only thing "onWallpaperChanged" does is call
hideWallpaperSetByMessage, I think it would be better to have that function
called for readability and maintainability. Right now it's not clear which
conditions it should be called in.

[bshe, 2014-06-18 21:07:42]

PTAL. Thanks!

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/338883004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:361:
this.hideWallpaperSetByMessage_();
make sense. Added a onWallpaperChanged function and added some common logic into
it.

On 2014/06/18 00:59:48, flackr wrote:
> On 2014/06/17 19:01:44, bshe wrote:
> > On 2014/06/17 15:20:34, flackr wrote:
> > > Rather than sprinkling this throughout the code, can you add a function
> which
> > is
> > > called when the wallpaper is changed? Perhaps onWallpaperChanged_.
> > 
> > Unfortunately, there are a few private APIs(setWallpaper,
setWallpaperIfExist,
> > setCustomWallpaper, resetWallpaper) that might change wallpaper. And after
> each
> > api call, we dont want to do the same thing. This makes adding a general
> > onWallpaperChanged_ function not trivial and possibly very complicate code.
Do
> > you mind if I keep it this way?
> 
> Even if the only thing "onWallpaperChanged" does is call
> hideWallpaperSetByMessage, I think it would be better to have that function
> called for readability and maintainability. Right now it's not clear which
> conditions it should be called in.

[flackr, 2014-06-18 21:26:58]

lgtm with nit

https://codereview.chromium.org/338883004/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/338883004/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:490:
currentWallpaperURL) {
nit: align, or wrap first arg.

[bshe, 2014-06-18 21:31:20]

thanks!

https://codereview.chromium.org/338883004/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/338883004/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:490:
currentWallpaperURL) {
On 2014/06/18 21:26:58, flackr wrote:
> nit: align, or wrap first arg.

Done.

[bshe, 2014-06-19 11:07:04]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-06-19 11:09:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bshe@chromium.org/338883004/80001

[commit-bot: I haz the power, 2014-06-19 13:12:53]

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

[commit-bot: I haz the power, 2014-06-19 13:12:53]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[bshe, 2014-06-19 13:19:29]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-06-19 13:20:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bshe@chromium.org/338883004/80001

[commit-bot: I haz the power, 2014-06-19 15:07:16]

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

[commit-bot: I haz the power, 2014-06-19 15:07:17]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[bshe, 2014-06-19 15:11:28]

The CQ bit was checked by bshe@chromium.org

[commit-bot: I haz the power, 2014-06-19 15:13:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bshe@chromium.org/338883004/80001

[commit-bot: I haz the power, 2014-06-19 16:28:05]

Change committed as 278399