Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1437833002:
Added check for avoid repeating variations first run seed fetch (Closed)
|
Created:
5 years, 1 month ago by Alexander Agulenko Modified:
5 years, 1 month ago CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded check for avoid repeating variations first run seed fetch
BUG=551029
Patch Set 1 #
Total comments: 2
Patch Set 2 : Implemented marking seed storing as successful from C++ side #
Total comments: 2
Messages
Total messages: 17 (4 generated)
Description was changed from ========== Added check for avoid repeating variations first run seed fetch BUG= ========== to ========== Added check for avoid repeating variations first run seed fetch BUG= ==========
agulenko@google.com changed reviewers: + asvitkine@chromium.org, holte@chromium.org
agulenko@google.com changed required reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:73: VariationsSeedBridge.markVariationsFirstRunSeedFetchSucceeded(getApplicationContext()); I'm not convinced this is the right place to do it. 1) What if it always fails? Then we keep trying each time. 2) What if it fails but later on regular variations code succeeds fetching it? Then this code will try to fetch this again. How about making the native code call out to Java when LoadSeed() succeeds loading a seed, which will mark this pref. Separately, the logic for VariationsSeedBridge.isVariationsFirstRunSeedFetchSucceeded() can check the actual seed content pref in addition to this pref. Something like (maybe with different names): if (sFetchInProgress || hasJavaPref() || hasNativePref()) where hasJavaPref() checks that we've fetched a pref while hasNativePref() checks whether the C++ code called us and told us that LoadSeed() succeeded.
where hasJavaPref() checks that we've fetched a pref while hasNativePref() checks whether the C++ code called us and told us that LoadSeed() succeeded. Do you suggest to call the function from C++ side every time when we succeeded with loading seed? Or just once when we import this seed from Java side? On Wed, Nov 11, 2015 at 7:04 AM <asvitkine@chromium.org> wrote: > > > https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... > File > > components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java > (right): > > > https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... > > components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:73 > <https://codereview.chromium.org/1437833002/diff/1/components/variations/andro...> > : > > VariationsSeedBridge.markVariationsFirstRunSeedFetchSucceeded(getApplicationContext()); > I'm not convinced this is the right place to do it. > > 1) What if it always fails? Then we keep trying each time. > 2) What if it fails but later on regular variations code succeeds > fetching it? Then this code will try to fetch this again. > > How about making the native code call out to Java when LoadSeed() > succeeds loading a seed, which will mark this pref. > > Separately, the logic for > VariationsSeedBridge.isVariationsFirstRunSeedFetchSucceeded() can check > the actual seed content pref in addition to this pref. > > Something like (maybe with different names): > > if (sFetchInProgress || hasJavaPref() || hasNativePref()) > > where hasJavaPref() checks that we've fetched a pref while > hasNativePref() checks whether the C++ code called us and told us that > LoadSeed() succeeded. > > https://codereview.chromium.org/1437833002/ > -- Alexander Agulenko -- 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.
Hmm, good question. Instead of from LoadSeed(), we should call it from StoreSeed() when it *first* stores a non-empty seed. (i.e. when we go from "no seed" -> "have seed") On Wed, Nov 11, 2015 at 2:51 PM, Alexander Agulenko <agulenko@google.com> wrote: > where hasJavaPref() checks that we've fetched a pref while > hasNativePref() checks whether the C++ code called us and told us that > LoadSeed() succeeded. > > Do you suggest to call the function from C++ side every time when we > succeeded with loading seed? Or just once when we import this seed from > Java side? > > On Wed, Nov 11, 2015 at 7:04 AM <asvitkine@chromium.org> wrote: > >> >> >> https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... >> File >> >> components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java >> (right): >> >> >> https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... >> >> components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:73 >> <https://codereview.chromium.org/1437833002/diff/1/components/variations/andro...> >> : >> >> VariationsSeedBridge.markVariationsFirstRunSeedFetchSucceeded(getApplicationContext()); >> I'm not convinced this is the right place to do it. >> >> 1) What if it always fails? Then we keep trying each time. >> 2) What if it fails but later on regular variations code succeeds >> fetching it? Then this code will try to fetch this again. >> >> How about making the native code call out to Java when LoadSeed() >> succeeds loading a seed, which will mark this pref. >> >> Separately, the logic for >> VariationsSeedBridge.isVariationsFirstRunSeedFetchSucceeded() can check >> the actual seed content pref in addition to this pref. >> >> Something like (maybe with different names): >> >> if (sFetchInProgress || hasJavaPref() || hasNativePref()) >> >> where hasJavaPref() checks that we've fetched a pref while >> hasNativePref() checks whether the C++ code called us and told us that >> LoadSeed() succeeded. >> >> https://codereview.chromium.org/1437833002/ >> > -- > Alexander Agulenko > -- 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.
Can I then add a new pref into local state file? On Wed, Nov 11, 2015 at 12:19 PM Alexei Svitkine <asvitkine@chromium.org> wrote: > Hmm, good question. > > Instead of from LoadSeed(), we should call it from StoreSeed() when it > *first* stores a non-empty seed. (i.e. when we go from "no seed" -> "have > seed") > > On Wed, Nov 11, 2015 at 2:51 PM, Alexander Agulenko <agulenko@google.com> > wrote: > >> where hasJavaPref() checks that we've fetched a pref while >> hasNativePref() checks whether the C++ code called us and told us that >> LoadSeed() succeeded. >> >> Do you suggest to call the function from C++ side every time when we >> succeeded with loading seed? Or just once when we import this seed from >> Java side? >> >> On Wed, Nov 11, 2015 at 7:04 AM <asvitkine@chromium.org> wrote: >> >>> >>> >>> https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... >>> File >>> >>> components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java >>> (right): >>> >>> >>> https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... >>> >>> components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:73 >>> <https://codereview.chromium.org/1437833002/diff/1/components/variations/andro...> >>> : >>> >>> VariationsSeedBridge.markVariationsFirstRunSeedFetchSucceeded(getApplicationContext()); >>> I'm not convinced this is the right place to do it. >>> >>> 1) What if it always fails? Then we keep trying each time. >>> 2) What if it fails but later on regular variations code succeeds >>> fetching it? Then this code will try to fetch this again. >>> >>> How about making the native code call out to Java when LoadSeed() >>> succeeds loading a seed, which will mark this pref. >>> >>> Separately, the logic for >>> VariationsSeedBridge.isVariationsFirstRunSeedFetchSucceeded() can check >>> the actual seed content pref in addition to this pref. >>> >>> Something like (maybe with different names): >>> >>> if (sFetchInProgress || hasJavaPref() || hasNativePref()) >>> >>> where hasJavaPref() checks that we've fetched a pref while >>> hasNativePref() checks whether the C++ code called us and told us that >>> LoadSeed() succeeded. >>> >>> https://codereview.chromium.org/1437833002/ >>> >> -- >> Alexander Agulenko >> > > -- Alexander Agulenko -- 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.
I mean something like new boolean pref which I will set as true after successful seed storing and calling the function on Java side. On Wed, Nov 11, 2015 at 12:22 PM Alexander Agulenko <agulenko@google.com> wrote: > Can I then add a new pref into local state file? > > On Wed, Nov 11, 2015 at 12:19 PM Alexei Svitkine <asvitkine@chromium.org> > wrote: > >> Hmm, good question. >> >> Instead of from LoadSeed(), we should call it from StoreSeed() when it >> *first* stores a non-empty seed. (i.e. when we go from "no seed" -> "have >> seed") >> >> On Wed, Nov 11, 2015 at 2:51 PM, Alexander Agulenko <agulenko@google.com> >> wrote: >> >>> where hasJavaPref() checks that we've fetched a pref while >>> hasNativePref() checks whether the C++ code called us and told us that >>> LoadSeed() succeeded. >>> >>> Do you suggest to call the function from C++ side every time when we >>> succeeded with loading seed? Or just once when we import this seed from >>> Java side? >>> >>> On Wed, Nov 11, 2015 at 7:04 AM <asvitkine@chromium.org> wrote: >>> >>>> >>>> >>>> https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... >>>> File >>>> >>>> components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java >>>> (right): >>>> >>>> >>>> https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... >>>> >>>> components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:73 >>>> <https://codereview.chromium.org/1437833002/diff/1/components/variations/andro...> >>>> : >>>> >>>> VariationsSeedBridge.markVariationsFirstRunSeedFetchSucceeded(getApplicationContext()); >>>> I'm not convinced this is the right place to do it. >>>> >>>> 1) What if it always fails? Then we keep trying each time. >>>> 2) What if it fails but later on regular variations code succeeds >>>> fetching it? Then this code will try to fetch this again. >>>> >>>> How about making the native code call out to Java when LoadSeed() >>>> succeeds loading a seed, which will mark this pref. >>>> >>>> Separately, the logic for >>>> VariationsSeedBridge.isVariationsFirstRunSeedFetchSucceeded() can check >>>> the actual seed content pref in addition to this pref. >>>> >>>> Something like (maybe with different names): >>>> >>>> if (sFetchInProgress || hasJavaPref() || hasNativePref()) >>>> >>>> where hasJavaPref() checks that we've fetched a pref while >>>> hasNativePref() checks whether the C++ code called us and told us that >>>> LoadSeed() succeeded. >>>> >>>> https://codereview.chromium.org/1437833002/ >>>> >>> -- >>> Alexander Agulenko >>> >> >> -- > Alexander Agulenko > -- Alexander Agulenko -- 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.
Why do you need a new pref? The client logic should just be "if (current
pref is empty && we're setting it) {notify java}".
On Wed, Nov 11, 2015 at 3:22 PM, Alexander Agulenko <agulenko@google.com>
wrote:
> Can I then add a new pref into local state file?
>
> On Wed, Nov 11, 2015 at 12:19 PM Alexei Svitkine <asvitkine@chromium.org>
> wrote:
>
>> Hmm, good question.
>>
>> Instead of from LoadSeed(), we should call it from StoreSeed() when it
>> *first* stores a non-empty seed. (i.e. when we go from "no seed" -> "have
>> seed")
>>
>> On Wed, Nov 11, 2015 at 2:51 PM, Alexander Agulenko <agulenko@google.com>
>> wrote:
>>
>>> where hasJavaPref() checks that we've fetched a pref while
>>> hasNativePref() checks whether the C++ code called us and told us that
>>> LoadSeed() succeeded.
>>>
>>> Do you suggest to call the function from C++ side every time when we
>>> succeeded with loading seed? Or just once when we import this seed from
>>> Java side?
>>>
>>> On Wed, Nov 11, 2015 at 7:04 AM <asvitkine@chromium.org> wrote:
>>>
>>>>
>>>>
>>>>
https://codereview.chromium.org/1437833002/diff/1/components/variations/andro...
>>>> File
>>>>
>>>>
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
>>>> (right):
>>>>
>>>>
>>>>
https://codereview.chromium.org/1437833002/diff/1/components/variations/andro...
>>>>
>>>>
components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:73
>>>>
<https://codereview.chromium.org/1437833002/diff/1/components/variations/andro...>
>>>> :
>>>>
>>>>
VariationsSeedBridge.markVariationsFirstRunSeedFetchSucceeded(getApplicationContext());
>>>> I'm not convinced this is the right place to do it.
>>>>
>>>> 1) What if it always fails? Then we keep trying each time.
>>>> 2) What if it fails but later on regular variations code succeeds
>>>> fetching it? Then this code will try to fetch this again.
>>>>
>>>> How about making the native code call out to Java when LoadSeed()
>>>> succeeds loading a seed, which will mark this pref.
>>>>
>>>> Separately, the logic for
>>>> VariationsSeedBridge.isVariationsFirstRunSeedFetchSucceeded() can check
>>>> the actual seed content pref in addition to this pref.
>>>>
>>>> Something like (maybe with different names):
>>>>
>>>> if (sFetchInProgress || hasJavaPref() || hasNativePref())
>>>>
>>>> where hasJavaPref() checks that we've fetched a pref while
>>>> hasNativePref() checks whether the C++ code called us and told us that
>>>> LoadSeed() succeeded.
>>>>
>>>> https://codereview.chromium.org/1437833002/
>>>>
>>> --
>>> Alexander Agulenko
>>>
>>
>> --
> Alexander Agulenko
>
--
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.
Okay, got it. But what then the function hasJavaPref() is? What is it supposed to do? On Wed, Nov 11, 2015 at 12:25 PM Alexei Svitkine <asvitkine@chromium.org> wrote: > Why do you need a new pref? The client logic should just be "if (current > pref is empty && we're setting it) {notify java}". > > On Wed, Nov 11, 2015 at 3:22 PM, Alexander Agulenko <agulenko@google.com> > wrote: > >> Can I then add a new pref into local state file? >> >> On Wed, Nov 11, 2015 at 12:19 PM Alexei Svitkine <asvitkine@chromium.org> >> wrote: >> >>> Hmm, good question. >>> >>> Instead of from LoadSeed(), we should call it from StoreSeed() when it >>> *first* stores a non-empty seed. (i.e. when we go from "no seed" -> "have >>> seed") >>> >>> On Wed, Nov 11, 2015 at 2:51 PM, Alexander Agulenko <agulenko@google.com >>> > wrote: >>> >>>> where hasJavaPref() checks that we've fetched a pref while >>>> hasNativePref() checks whether the C++ code called us and told us that >>>> LoadSeed() succeeded. >>>> >>>> Do you suggest to call the function from C++ side every time when we >>>> succeeded with loading seed? Or just once when we import this seed from >>>> Java side? >>>> >>>> On Wed, Nov 11, 2015 at 7:04 AM <asvitkine@chromium.org> wrote: >>>> >>>>> >>>>> >>>>> https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... >>>>> File >>>>> >>>>> components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java >>>>> (right): >>>>> >>>>> >>>>> https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... >>>>> >>>>> components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:73 >>>>> <https://codereview.chromium.org/1437833002/diff/1/components/variations/andro...> >>>>> : >>>>> >>>>> VariationsSeedBridge.markVariationsFirstRunSeedFetchSucceeded(getApplicationContext()); >>>>> I'm not convinced this is the right place to do it. >>>>> >>>>> 1) What if it always fails? Then we keep trying each time. >>>>> 2) What if it fails but later on regular variations code succeeds >>>>> fetching it? Then this code will try to fetch this again. >>>>> >>>>> How about making the native code call out to Java when LoadSeed() >>>>> succeeds loading a seed, which will mark this pref. >>>>> >>>>> Separately, the logic for >>>>> VariationsSeedBridge.isVariationsFirstRunSeedFetchSucceeded() can check >>>>> the actual seed content pref in addition to this pref. >>>>> >>>>> Something like (maybe with different names): >>>>> >>>>> if (sFetchInProgress || hasJavaPref() || hasNativePref()) >>>>> >>>>> where hasJavaPref() checks that we've fetched a pref while >>>>> hasNativePref() checks whether the C++ code called us and told us that >>>>> LoadSeed() succeeded. >>>>> >>>>> https://codereview.chromium.org/1437833002/ >>>>> >>>> -- >>>> Alexander Agulenko >>>> >>> >>> -- >> Alexander Agulenko >> > > -- Alexander Agulenko -- 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.
Description was changed from ========== Added check for avoid repeating variations first run seed fetch BUG= ========== to ========== Added check for avoid repeating variations first run seed fetch BUG=551029 ==========
https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1437833002/diff/1/components/variations/andro... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:73: VariationsSeedBridge.markVariationsFirstRunSeedFetchSucceeded(getApplicationContext()); On 2015/11/11 15:04:27, Alexei Svitkine (slow) wrote: > I'm not convinced this is the right place to do it. > > 1) What if it always fails? Then we keep trying each time. > 2) What if it fails but later on regular variations code succeeds fetching it? > Then this code will try to fetch this again. > > How about making the native code call out to Java when LoadSeed() succeeds > loading a seed, which will mark this pref. > > Separately, the logic for > VariationsSeedBridge.isVariationsFirstRunSeedFetchSucceeded() can check the > actual seed content pref in addition to this pref. > > Something like (maybe with different names): > > if (sFetchInProgress || hasJavaPref() || hasNativePref()) > > where hasJavaPref() checks that we've fetched a pref while hasNativePref() > checks whether the C++ code called us and told us that LoadSeed() succeeded. Done.
https://codereview.chromium.org/1437833002/diff/20001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1437833002/diff/20001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:46: sFetchInProgress = false; Where do you set sFetchInProgress = true?
https://codereview.chromium.org/1437833002/diff/20001/components/variations/a... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java (right): https://codereview.chromium.org/1437833002/diff/20001/components/variations/a... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:46: sFetchInProgress = false; On 2015/11/12 19:33:19, Steven Holte wrote: > Where do you set sFetchInProgress = true? Seems like I occasionally removed it (maybe simply forgot). Done. This CL is now merged to https://codereview.chromium.org/1438123002/, let's move there (I will close this issue).
Message was sent while issue was closed.
On 2015/11/12 22:06:48, Alexander Agulenko wrote: > https://codereview.chromium.org/1437833002/diff/20001/components/variations/a... > File > components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java > (right): > > https://codereview.chromium.org/1437833002/diff/20001/components/variations/a... > components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java:46: > sFetchInProgress = false; > On 2015/11/12 19:33:19, Steven Holte wrote: > > Where do you set sFetchInProgress = true? > > Seems like I occasionally removed it (maybe simply forgot). > > Done. > > This CL is now merged to https://codereview.chromium.org/1438123002/, let's move > there (I will close this issue). Sorry, the correct merged issue is https://codereview.chromium.org/1437833004/. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
