Show feedback button on sad tab if sad tab has shown recently.

Show feedback button on sad tab if sad tab has shown recently.

BUG=713828
TEST=manual, see bug.

Review-Url: https://codereview.chromium.org/2885143009
Cr-Commit-Position: refs/heads/master@{#473204}
Committed: https://chromium.googlesource.com/chromium/src/+/c9a40db46670d1c37111c0010d36fc290566eaf0

[Will Harris, 2017-05-18 00:56:04]

Description was changed from

==========
Show feedback button on sad tab if sad tab has shown recently.

BUG=713828
==========

to

==========
Show feedback button on sad tab if sad tab has shown recently.

BUG=713828
==========

[Will Harris, 2017-05-18 00:56:04]

wfh@chromium.org changed reviewers:
+ sky@chromium.org

[Will Harris, 2017-05-18 00:56:13]

Description was changed from

==========
Show feedback button on sad tab if sad tab has shown recently.

BUG=713828
==========

to

==========
Show feedback button on sad tab if sad tab has shown recently.

BUG=713828
TEST=manual, see bug.
==========

[Will Harris, 2017-05-18 00:57:48]

PTAL

[Will Harris, 2017-05-18 00:58:08]

and a question

https://codereview.chromium.org/2885143009/diff/1/chrome/browser/ui/sad_tab.cc
File chrome/browser/ui/sad_tab.cc (right):

https://codereview.chromium.org/2885143009/diff/1/chrome/browser/ui/sad_tab.c...
chrome/browser/ui/sad_tab.cc:65: static base::TimeTicks last_called =
base::TimeTicks::UnixEpoch();
static initalizer for this class is allowed?

[sky, 2017-05-18 16:37:58]

+thakis for static initializer question.

https://codereview.chromium.org/2885143009/diff/1/chrome/browser/ui/sad_tab.cc
File chrome/browser/ui/sad_tab.cc (right):

https://codereview.chromium.org/2885143009/diff/1/chrome/browser/ui/sad_tab.c...
chrome/browser/ui/sad_tab.cc:65: static base::TimeTicks last_called =
base::TimeTicks::UnixEpoch();
On 2017/05/18 00:58:08, Will Harris wrote:
> static initalizer for this class is allowed?

Not sure on that, Nico likely knows.

[Will Harris, 2017-05-18 16:46:14]

wfh@chromium.org changed reviewers:
+ thakis@chromium.org

[Will Harris, 2017-05-18 16:46:21]

+thakis for realz

[Nico, 2017-05-18 16:54:58]

https://codereview.chromium.org/2885143009/diff/1/chrome/browser/ui/sad_tab.cc
File chrome/browser/ui/sad_tab.cc (right):

https://codereview.chromium.org/2885143009/diff/1/chrome/browser/ui/sad_tab.c...
chrome/browser/ui/sad_tab.cc:65: static base::TimeTicks last_called =
base::TimeTicks::UnixEpoch();
On 2017/05/18 16:37:58, sky wrote:
> On 2017/05/18 00:58:08, Will Harris wrote:
> > static initalizer for this class is allowed?
> 
> Not sure on that, Nico likely knows.

This is a function level static. Despite the similar name, these don't need a
static initializers (which runs before main). Instead, it gets initialized the
first time this function runs. (The compiler emits a hidden global bool to store
if it's been initialized yet)

[sky, 2017-05-18 17:01:34]

But then we have a static destructor, right? Which the style guide says we
shouldn't use.

On Thu, May 18, 2017 at 9:54 AM, <thakis@chromium.org> wrote:

>
> https://codereview.chromium.org/2885143009/diff/1/chrome/
> browser/ui/sad_tab.cc
> File chrome/browser/ui/sad_tab.cc (right):
>
> https://codereview.chromium.org/2885143009/diff/1/chrome/
> browser/ui/sad_tab.cc#newcode65
> chrome/browser/ui/sad_tab.cc:65: static base::TimeTicks last_called =
> base::TimeTicks::UnixEpoch();
> On 2017/05/18 16:37:58, sky wrote:
> > On 2017/05/18 00:58:08, Will Harris wrote:
> > > static initalizer for this class is allowed?
> >
> > Not sure on that, Nico likely knows.
>
> This is a function level static. Despite the similar name, these don't
> need a static initializers (which runs before main). Instead, it gets
> initialized the first time this function runs. (The compiler emits a
> hidden global bool to store if it's been initialized yet)
>
> https://codereview.chromium.org/2885143009/
>

-- 
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.

[Nico, 2017-05-18 17:41:59]

Yes, if this class has a dtor, that's correct. CR_DEFINE_STATIC_LOCAL
prevents that by going to the heap. Maybe you can use that.

On May 18, 2017 1:01 PM, "Scott Violet" <sky@chromium.org> wrote:

> But then we have a static destructor, right? Which the style guide says we
> shouldn't use.
>
> On Thu, May 18, 2017 at 9:54 AM, <thakis@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/2885143009/diff/1/chrome/bro
>> wser/ui/sad_tab.cc
>> File chrome/browser/ui/sad_tab.cc (right):
>>
>> https://codereview.chromium.org/2885143009/diff/1/chrome/bro
>> wser/ui/sad_tab.cc#newcode65
>> chrome/browser/ui/sad_tab.cc:65: static base::TimeTicks last_called =
>> base::TimeTicks::UnixEpoch();
>> On 2017/05/18 16:37:58, sky wrote:
>> > On 2017/05/18 00:58:08, Will Harris wrote:
>> > > static initalizer for this class is allowed?
>> >
>> > Not sure on that, Nico likely knows.
>>
>> This is a function level static. Despite the similar name, these don't
>> need a static initializers (which runs before main). Instead, it gets
>> initialized the first time this function runs. (The compiler emits a
>> hidden global bool to store if it's been initialized yet)
>>
>> https://codereview.chromium.org/2885143009/
>>
>
>

-- 
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.

[Will Harris, 2017-05-18 17:44:44]

On 2017/05/18 17:41:59, Nico wrote:
> Yes, if this class has a dtor, that's correct. CR_DEFINE_STATIC_LOCAL
> prevents that by going to the heap. Maybe you can use that.
> 
> On May 18, 2017 1:01 PM, "Scott Violet" <mailto:sky@chromium.org> wrote:
> 
> > But then we have a static destructor, right? Which the style guide says we
> > shouldn't use.
> >
> > On Thu, May 18, 2017 at 9:54 AM, <mailto:thakis@chromium.org> wrote:
> >
> >>
> >> https://codereview.chromium.org/2885143009/diff/1/chrome/bro
> >> wser/ui/sad_tab.cc
> >> File chrome/browser/ui/sad_tab.cc (right):
> >>
> >> https://codereview.chromium.org/2885143009/diff/1/chrome/bro
> >> wser/ui/sad_tab.cc#newcode65
> >> chrome/browser/ui/sad_tab.cc:65: static base::TimeTicks last_called =
> >> base::TimeTicks::UnixEpoch();
> >> On 2017/05/18 16:37:58, sky wrote:
> >> > On 2017/05/18 00:58:08, Will Harris wrote:
> >> > > static initalizer for this class is allowed?
> >> >
> >> > Not sure on that, Nico likely knows.
> >>
> >> This is a function level static. Despite the similar name, these don't
> >> need a static initializers (which runs before main). Instead, it gets
> >> initialized the first time this function runs. (The compiler emits a
> >> hidden global bool to store if it's been initialized yet)
> >>
> >> https://codereview.chromium.org/2885143009/
> >>
> >
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

I could just call ToInternalValue() on base::TimeTicks and store a static
uint64_t instead. This avoids any of this mess, but I didn't do that because
timeticks.h warns against not using it. WDYT?

[sky, 2017-05-18 19:40:55]

On 2017/05/18 17:44:44, Will Harris wrote:
> On 2017/05/18 17:41:59, Nico wrote:
> > Yes, if this class has a dtor, that's correct. CR_DEFINE_STATIC_LOCAL
> > prevents that by going to the heap. Maybe you can use that.
> > 
> > On May 18, 2017 1:01 PM, "Scott Violet" <mailto:sky@chromium.org> wrote:
> > 
> > > But then we have a static destructor, right? Which the style guide says we
> > > shouldn't use.
> > >
> > > On Thu, May 18, 2017 at 9:54 AM, <mailto:thakis@chromium.org> wrote:
> > >
> > >>
> > >> https://codereview.chromium.org/2885143009/diff/1/chrome/bro
> > >> wser/ui/sad_tab.cc
> > >> File chrome/browser/ui/sad_tab.cc (right):
> > >>
> > >> https://codereview.chromium.org/2885143009/diff/1/chrome/bro
> > >> wser/ui/sad_tab.cc#newcode65
> > >> chrome/browser/ui/sad_tab.cc:65: static base::TimeTicks last_called =
> > >> base::TimeTicks::UnixEpoch();
> > >> On 2017/05/18 16:37:58, sky wrote:
> > >> > On 2017/05/18 00:58:08, Will Harris wrote:
> > >> > > static initalizer for this class is allowed?
> > >> >
> > >> > Not sure on that, Nico likely knows.
> > >>
> > >> This is a function level static. Despite the similar name, these don't
> > >> need a static initializers (which runs before main). Instead, it gets
> > >> initialized the first time this function runs. (The compiler emits a
> > >> hidden global bool to store if it's been initialized yet)
> > >>
> > >> https://codereview.chromium.org/2885143009/
> > >>
> > >
> > >
> > 
> > -- 
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> I could just call ToInternalValue() on base::TimeTicks and store a static
> uint64_t instead. This avoids any of this mess, but I didn't do that because
> timeticks.h warns against not using it. WDYT?

The warning is not to do arithmetic against it. As long you create a TimeTicks
from the int64_t value and do the arithmetic on it, then I think you're fine.

[Will Harris, 2017-05-18 20:56:51]

On 2017/05/18 19:40:55, sky wrote:
> On 2017/05/18 17:44:44, Will Harris wrote:
> > On 2017/05/18 17:41:59, Nico wrote:
> > > Yes, if this class has a dtor, that's correct. CR_DEFINE_STATIC_LOCAL
> > > prevents that by going to the heap. Maybe you can use that.
> > > 
> > > On May 18, 2017 1:01 PM, "Scott Violet" <mailto:sky@chromium.org> wrote:
> > > 
> > > > But then we have a static destructor, right? Which the style guide says
we
> > > > shouldn't use.
> > > >
> > > > On Thu, May 18, 2017 at 9:54 AM, <mailto:thakis@chromium.org> wrote:
> > > >
> > > >>
> > > >> https://codereview.chromium.org/2885143009/diff/1/chrome/bro
> > > >> wser/ui/sad_tab.cc
> > > >> File chrome/browser/ui/sad_tab.cc (right):
> > > >>
> > > >> https://codereview.chromium.org/2885143009/diff/1/chrome/bro
> > > >> wser/ui/sad_tab.cc#newcode65
> > > >> chrome/browser/ui/sad_tab.cc:65: static base::TimeTicks last_called =
> > > >> base::TimeTicks::UnixEpoch();
> > > >> On 2017/05/18 16:37:58, sky wrote:
> > > >> > On 2017/05/18 00:58:08, Will Harris wrote:
> > > >> > > static initalizer for this class is allowed?
> > > >> >
> > > >> > Not sure on that, Nico likely knows.
> > > >>
> > > >> This is a function level static. Despite the similar name, these don't
> > > >> need a static initializers (which runs before main). Instead, it gets
> > > >> initialized the first time this function runs. (The compiler emits a
> > > >> hidden global bool to store if it's been initialized yet)
> > > >>
> > > >> https://codereview.chromium.org/2885143009/
> > > >>
> > > >
> > > >
> > > 
> > > -- 
> > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> > I could just call ToInternalValue() on base::TimeTicks and store a static
> > uint64_t instead. This avoids any of this mess, but I didn't do that because
> > timeticks.h warns against not using it. WDYT?
> 
> The warning is not to do arithmetic against it. As long you create a TimeTicks
> from the int64_t value and do the arithmetic on it, then I think you're fine.

the constructor to create a TimeTicks from an int64_t is private... :(

[sky, 2017-05-18 22:47:06]

FromInternalValue?

On Thu, May 18, 2017 at 1:56 PM, <wfh@chromium.org> wrote:

> On 2017/05/18 19:40:55, sky wrote:
> > On 2017/05/18 17:44:44, Will Harris wrote:
> > > On 2017/05/18 17:41:59, Nico wrote:
> > > > Yes, if this class has a dtor, that's correct. CR_DEFINE_STATIC_LOCAL
> > > > prevents that by going to the heap. Maybe you can use that.
> > > >
> > > > On May 18, 2017 1:01 PM, "Scott Violet" <mailto:sky@chromium.org>
> wrote:
> > > >
> > > > > But then we have a static destructor, right? Which the style guide
> says
> we
> > > > > shouldn't use.
> > > > >
> > > > > On Thu, May 18, 2017 at 9:54 AM, <mailto:thakis@chromium.org>
> wrote:
> > > > >
> > > > >>
> > > > >> https://codereview.chromium.org/2885143009/diff/1/chrome/bro
> > > > >> wser/ui/sad_tab.cc
> > > > >> File chrome/browser/ui/sad_tab.cc (right):
> > > > >>
> > > > >> https://codereview.chromium.org/2885143009/diff/1/chrome/bro
> > > > >> wser/ui/sad_tab.cc#newcode65
> > > > >> chrome/browser/ui/sad_tab.cc:65: static base::TimeTicks
> last_called =
> > > > >> base::TimeTicks::UnixEpoch();
> > > > >> On 2017/05/18 16:37:58, sky wrote:
> > > > >> > On 2017/05/18 00:58:08, Will Harris wrote:
> > > > >> > > static initalizer for this class is allowed?
> > > > >> >
> > > > >> > Not sure on that, Nico likely knows.
> > > > >>
> > > > >> This is a function level static. Despite the similar name, these
> don't
> > > > >> need a static initializers (which runs before main). Instead, it
> gets
> > > > >> initialized the first time this function runs. (The compiler
> emits a
> > > > >> hidden global bool to store if it's been initialized yet)
> > > > >>
> > > > >> https://codereview.chromium.org/2885143009/
> > > > >>
> > > > >
> > > > >
> > > >
> > > > --
> > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > > I could just call ToInternalValue() on base::TimeTicks and store a
> static
> > > uint64_t instead. This avoids any of this mess, but I didn't do that
> because
> > > timeticks.h warns against not using it. WDYT?
> >
> > The warning is not to do arithmetic against it. As long you create a
> TimeTicks
> > from the int64_t value and do the arithmetic on it, then I think you're
> fine.
>
> the constructor to create a TimeTicks from an int64_t is private... :(
>
>
> https://codereview.chromium.org/2885143009/
>

-- 
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.

[Will Harris, 2017-05-18 22:50:17]

On 2017/05/18 22:47:06, sky wrote:
> FromInternalValue?

ah doh good spot, I only found the constructor not the static method

[Will Harris, 2017-05-18 22:59:48]

PTAL

https://codereview.chromium.org/2885143009/diff/1/chrome/browser/ui/sad_tab.cc
File chrome/browser/ui/sad_tab.cc (right):

https://codereview.chromium.org/2885143009/diff/1/chrome/browser/ui/sad_tab.c...
chrome/browser/ui/sad_tab.cc:65: static base::TimeTicks last_called =
base::TimeTicks::UnixEpoch();
On 2017/05/18 16:54:58, Nico wrote:
> On 2017/05/18 16:37:58, sky wrote:
> > On 2017/05/18 00:58:08, Will Harris wrote:
> > > static initalizer for this class is allowed?
> > 
> > Not sure on that, Nico likely knows.
> 
> This is a function level static. Despite the similar name, these don't need a
> static initializers (which runs before main). Instead, it gets initialized the
> first time this function runs. (The compiler emits a hidden global bool to
store
> if it's been initialized yet)

Acknowledged. Changed to store the int64_t static instead.

[Will Harris, 2017-05-19 06:41:06]

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

[commit-bot: I haz the power, 2017-05-19 06:41:44]

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-05-19 08:40:35]

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

[commit-bot: I haz the power, 2017-05-19 08:40:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sky, 2017-05-19 14:10:19]

LGTM

[Will Harris, 2017-05-19 15:15:45]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2017-05-19 15:16:09]

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-05-19 15:54:05]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495206945178050,
"parent_rev": "7586aa67fae852113735474c358205cef090e843", "commit_rev":
"c9a40db46670d1c37111c0010d36fc290566eaf0"}

[commit-bot: I haz the power, 2017-05-19 15:54:16]

Description was changed from

==========
Show feedback button on sad tab if sad tab has shown recently.

BUG=713828
TEST=manual, see bug.
==========

to

==========
Show feedback button on sad tab if sad tab has shown recently.

BUG=713828
TEST=manual, see bug.

Review-Url: https://codereview.chromium.org/2885143009
Cr-Commit-Position: refs/heads/master@{#473204}
Committed:
https://chromium.googlesource.com/chromium/src/+/c9a40db46670d1c37111c0010d36...
==========

[commit-bot: I haz the power, 2017-05-19 15:54:20]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c9a40db46670d1c37111c0010d36...