Fetch a new GAIA picture every time chrome://settings/manageProfile comes up.

Fetch a new GAIA picture every time chrome://settings/manageProfile comes up.

(currently we do it once, when a profile is signed in, and henceforth every 24h)

This assumes the --google-profile-info flag is in use (which comes for free
as part of --new-profile-management), and is similar to CrOS, which also forces
an update every time chrome://settings/changePicture is loaded.

I've also deleted the code that forced the "is using the gaia photo" flag to true
on every completed download, as it meant that we'd overwrite any custom avatars
the user may have chosen.

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

[noms (inactive), 2014-05-07 19:14:36]

Hiya!

There's an issue currently open where we are not re-downloading the GAIA profile
avatar often enough. Since --new-profile-management is moving forward towards
using the GAIA picture all of the time, we should probably update this photo
every time the user tries to change the profile's avatar. Please take a look.
Thanks!

Rachel: c/b/profiles
Evan: c/b/resources/, c/b/ui/. I added the call to refresh in each handler
separately, rather than in ManageProfileOverlay.showManageDialog(), because that
function also gets called every time the list of avatars changes (i.e. when the
new photo downloaded successfully), and that leads to an infinite loop.

[rpetterson, 2014-05-07 22:35:49]

profiles LGTM

[Evan Stade, 2014-05-07 22:38:55]

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:251:
chrome.send('refreshGAIAPicture');
why isn't this in ManageProfileOverlay.showManageDialog?

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/manage_profile_handler.h (right):

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/manage_profile_handler.h:127: void
RefreshGAIAPicture(const base::ListValue* args);
nit: initializations like GAIA should not be all caps in symbols (such as this
fn name). RefreshGaiaPicture() instead.

[noms (inactive), 2014-05-08 01:02:29]

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:251:
chrome.send('refreshGAIAPicture');
showManageDialog is also called by setProfilesInfo_, which is called when the
ProfileInfoCache updates (i.e. after the gaia picture has been refreshed). So
moving it to there leads to an infinite loop. 

On 2014/05/07 22:38:55, Evan Stade wrote:
> why isn't this in ManageProfileOverlay.showManageDialog?

[Evan Stade, 2014-05-08 01:09:57]

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:251:
chrome.send('refreshGAIAPicture');
On 2014/05/08 01:02:29, Monica Dinculescu wrote:
> showManageDialog is also called by setProfilesInfo_, which is called when the
> ProfileInfoCache updates (i.e. after the gaia picture has been refreshed). So
> moving it to there leads to an infinite loop. 
> 
> On 2014/05/07 22:38:55, Evan Stade wrote:
> > why isn't this in ManageProfileOverlay.showManageDialog?
> 

seems like the info cache shouldn't be updated if the gaia picture hasn't
actually changed.

[noms (inactive), 2014-05-08 15:29:05]

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:251:
chrome.send('refreshGAIAPicture');
We would have to do a pixel-by-pixel check when we re-download it, which we
currently don't. Basically, the GaiaInfoUpdateService just re-downloads the file
from a web service (which doesn't tell you when the file has been updated, to my
knowledge), and the ProfileInfoCache replaces the existing one with the new one.


On 2014/05/08 01:09:57, Evan Stade wrote:
> On 2014/05/08 01:02:29, Monica Dinculescu wrote:
> > showManageDialog is also called by setProfilesInfo_, which is called when
the
> > ProfileInfoCache updates (i.e. after the gaia picture has been refreshed).
So
> > moving it to there leads to an infinite loop. 
> > 
> > On 2014/05/07 22:38:55, Evan Stade wrote:
> > > why isn't this in ManageProfileOverlay.showManageDialog?
> > 
> 
> seems like the info cache shouldn't be updated if the gaia picture hasn't
> actually changed.

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/manage_profile_handler.h (right):

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/manage_profile_handler.h:127: void
RefreshGAIAPicture(const base::ListValue* args);
On 2014/05/07 22:38:55, Evan Stade wrote:
> nit: initializations like GAIA should not be all caps in symbols (such as this
> fn name). RefreshGaiaPicture() instead.

Done.

[Evan Stade, 2014-05-14 03:09:54]

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:251:
chrome.send('refreshGAIAPicture');
On 2014/05/08 15:29:06, Monica (OOO until May 15) wrote:
> We would have to do a pixel-by-pixel check when we re-download it, which we
> currently don't. Basically, the GaiaInfoUpdateService just re-downloads the
file
> from a web service (which doesn't tell you when the file has been updated, to
my
> knowledge), and the ProfileInfoCache replaces the existing one with the new
one.

Are you suggesting that checking the pixels is going to be too slow? It's just a
memcmp. Isn't sending the pixels over various wires to the renderer, repainting
the screen, etc., even slower?

> 
> 
> On 2014/05/08 01:09:57, Evan Stade wrote:
> > On 2014/05/08 01:02:29, Monica Dinculescu wrote:
> > > showManageDialog is also called by setProfilesInfo_, which is called when
> the
> > > ProfileInfoCache updates (i.e. after the gaia picture has been refreshed).
> So
> > > moving it to there leads to an infinite loop. 

I don't see a showManageDialog call in setProfilesInfo_. In any case, if we want
to get a new gaia picture when the dialog is shown, I think this code is too
fragile.

> > > 
> > > On 2014/05/07 22:38:55, Evan Stade wrote:
> > > > why isn't this in ManageProfileOverlay.showManageDialog?
> > > 
> > 
> > seems like the info cache shouldn't be updated if the gaia picture hasn't
> > actually changed.
>

[noms (inactive), 2014-05-20 20:08:57]

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:251:
chrome.send('refreshGAIAPicture');
setProfilesInfo_ calls ManageProfileOverlay.showManageDialog() if the dialog is
already showing:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

The people get api does not have a "last updated" field for the profile photo,
so if we want to make sure we are using the user's latest photo, we need to
always re-fetch it when editing the profile, i.e. showManageDialog is called. 

I will add the "if it's the same image don't update the info cache" code, and
rename the chrome signal to maybeRefreshGaiaPicture. Does that sound ok?


On 2014/05/14 03:09:55, Evan Stade wrote:
> On 2014/05/08 15:29:06, Monica (OOO until May 15) wrote:
> > We would have to do a pixel-by-pixel check when we re-download it, which we
> > currently don't. Basically, the GaiaInfoUpdateService just re-downloads the
> file
> > from a web service (which doesn't tell you when the file has been updated,
to
> my
> > knowledge), and the ProfileInfoCache replaces the existing one with the new
> one.
> 
> Are you suggesting that checking the pixels is going to be too slow? It's just
a
> memcmp. Isn't sending the pixels over various wires to the renderer,
repainting
> the screen, etc., even slower?
> 
> > 
> > 
> > On 2014/05/08 01:09:57, Evan Stade wrote:
> > > On 2014/05/08 01:02:29, Monica Dinculescu wrote:
> > > > showManageDialog is also called by setProfilesInfo_, which is called
when
> > the
> > > > ProfileInfoCache updates (i.e. after the gaia picture has been
refreshed).
> > So
> > > > moving it to there leads to an infinite loop. 
> 
> I don't see a showManageDialog call in setProfilesInfo_. In any case, if we
want
> to get a new gaia picture when the dialog is shown, I think this code is too
> fragile.
> 
> > > > 
> > > > On 2014/05/07 22:38:55, Evan Stade wrote:
> > > > > why isn't this in ManageProfileOverlay.showManageDialog?
> > > > 
> > > 
> > > seems like the info cache shouldn't be updated if the gaia picture hasn't
> > > actually changed.
> > 
>

[noms (inactive), 2014-05-21 17:27:38]

Good news! I did some more digging. and it turns out we don't have to check if
it's the same picture, as the ProfileDownloader 
does that. Basically, if you try to download the Gaia picture and you get the
same one you already have, the ProfileDownloader 
returns a status of ProfileDownloader::PICTURE_CACHED, and the
GaiaInfoUpdateService does not update the ProfileInfoCache 
(which means the ManageDialog does not re-render the same image).

Can you review the CL again please, given this new information? Thanks!

On 2014/05/20 20:08:57, Monica Dinculescu wrote:
>
https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
> File chrome/browser/resources/options/browser_options.js (right):
> 
>
https://codereview.chromium.org/267253004/diff/40001/chrome/browser/resources...
> chrome/browser/resources/options/browser_options.js:251:
> chrome.send('refreshGAIAPicture');
> setProfilesInfo_ calls ManageProfileOverlay.showManageDialog() if the dialog
is
> already showing:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
> The people get api does not have a "last updated" field for the profile photo,
> so if we want to make sure we are using the user's latest photo, we need to
> always re-fetch it when editing the profile, i.e. showManageDialog is called. 
> 
> I will add the "if it's the same image don't update the info cache" code, and
> rename the chrome signal to maybeRefreshGaiaPicture. Does that sound ok?
> 
> 
> On 2014/05/14 03:09:55, Evan Stade wrote:
> > On 2014/05/08 15:29:06, Monica (OOO until May 15) wrote:
> > > We would have to do a pixel-by-pixel check when we re-download it, which
we
> > > currently don't. Basically, the GaiaInfoUpdateService just re-downloads
the
> > file
> > > from a web service (which doesn't tell you when the file has been updated,
> to
> > my
> > > knowledge), and the ProfileInfoCache replaces the existing one with the
new
> > one.
> > 
> > Are you suggesting that checking the pixels is going to be too slow? It's
just
> a
> > memcmp. Isn't sending the pixels over various wires to the renderer,
> repainting
> > the screen, etc., even slower?
> > 
> > > 
> > > 
> > > On 2014/05/08 01:09:57, Evan Stade wrote:
> > > > On 2014/05/08 01:02:29, Monica Dinculescu wrote:
> > > > > showManageDialog is also called by setProfilesInfo_, which is called
> when
> > > the
> > > > > ProfileInfoCache updates (i.e. after the gaia picture has been
> refreshed).
> > > So
> > > > > moving it to there leads to an infinite loop. 
> > 
> > I don't see a showManageDialog call in setProfilesInfo_. In any case, if we
> want
> > to get a new gaia picture when the dialog is shown, I think this code is too
> > fragile.
> > 
> > > > > 
> > > > > On 2014/05/07 22:38:55, Evan Stade wrote:
> > > > > > why isn't this in ManageProfileOverlay.showManageDialog?
> > > > > 
> > > > 
> > > > seems like the info cache shouldn't be updated if the gaia picture
hasn't
> > > > actually changed.
> > > 
> >

[Evan Stade, 2014-05-21 23:28:29]

https://codereview.chromium.org/267253004/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/267253004/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:251:
chrome.send('refreshGaiaPicture');
I still do not think this belongs here. It is too easy to add a new way to show
the manage dialog without adding this line as well. The only reason for the not
re-updating yak shave was to avoid the infinite loop. Please find a way to fix
the infinite loop.

[noms (inactive), 2014-05-23 15:09:47]

https://codereview.chromium.org/267253004/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/267253004/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:251:
chrome.send('refreshGaiaPicture');
Done! There's no infinite loop anymore, so I'm now suspicious it was either a)
entirely unrelated in ToT or b) I had written some nonsensical code before
coffee that made no sense. :( 

Super sorry for this! 

On 2014/05/21 23:28:30, Evan Stade wrote:
> I still do not think this belongs here. It is too easy to add a new way to
show
> the manage dialog without adding this line as well. The only reason for the
not
> re-updating yak shave was to avoid the infinite loop. Please find a way to fix
> the infinite loop.

[Evan Stade, 2014-05-23 15:33:22]

webui and resources lgtm

[noms (inactive), 2014-05-23 15:35:15]

Thanks! Sorry again for the nonsense! :(

[noms (inactive), 2014-05-23 15:35:19]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-05-23 15:36:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/267253004/90001

[Evan Stade, 2014-05-23 16:00:53]

On 2014/05/23 15:35:15, Monica Dinculescu wrote:
> Thanks! Sorry again for the nonsense! :(

not a problem

[commit-bot: I haz the power, 2014-05-23 20:22:14]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-23 22:17:18]

Change committed as 272614