Interstitial options are not OR'd properly.

Interstitial options are not OR'd properly.

Instead of bitwise ORing the interstitial options, we were simply
assigning new values. This CL changes the assignment to a proper bitwise
OR so all options are maintained.

R=meacer@chromium.org,
TBR=jam@chromium.org,creis@chromium.org
BUG=415256
Committed: https://crrev.com/3a18d0b750ce048eed897ca3e22a3bccaec2fd3c
Cr-Commit-Position: refs/heads/master@{#295807}

[jww, 2014-09-18 17:57:00]

jww@chromium.org changed reviewers:
+ meacer@chromium.org

[meacer, 2014-09-18 18:03:33]

Lgtm

https://codereview.chromium.org/578373002/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/578373002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1715: options_mask |=
SSLBlockingPage::OVERRIDABLE;
Looks like overridable, strict_enforcement and expired_previous_decision are
extracted from another options_mask at
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ss...

So perhaps you can just pass options_mask all the way from there to here?

[jww, 2014-09-18 20:11:18]

jww@chromium.org changed reviewers:
+ jam@chromium.org

[jww, 2014-09-18 20:11:19]

jam@, can you take a look at content/ and browser/? Should be a pretty
straightforward change.

https://codereview.chromium.org/578373002/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/578373002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1715: options_mask |=
SSLBlockingPage::OVERRIDABLE;
On 2014/09/18 18:03:33, Mustafa Emre Acer wrote:
> Looks like overridable, strict_enforcement and expired_previous_decision are
> extracted from another options_mask at
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ss...
> 
> So perhaps you can just pass options_mask all the way from there to here?
Yup, I like the suggestion. Done.

[jww, 2014-09-19 17:25:05]

jww@chromium.org changed reviewers:
- jam@chromium.org

[jww, 2014-09-19 17:26:30]

jww@chromium.org changed reviewers:
+ creis@chromium.org

[jww, 2014-09-19 17:27:30]

creis@, would you mind taking a look at content/? It appears that jam@ is OOO,
so I'm TBR'ing him on this, but I'd still like to get an OWNER to look at
content/. Thanks!

[Charlie Reis, 2014-09-19 20:00:11]

creis@chromium.org changed reviewers:
+ jam@chromium.org

[Charlie Reis, 2014-09-19 20:00:12]

The tradeoff here is between "fanning out" the mask into 3 bools in the public
API vs. introducing a new public enum that both content/ and chrome/ can use.  I
think this is a reasonable change, but I do want John's approval before we land
it (since he's the guardian of the API).

https://codereview.chromium.org/578373002/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/578373002/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1715: options_mask =
SSLBlockingPage::OVERRIDABLE;
Yikes.  Yeah, this isn't much of a bitmask.

https://codereview.chromium.org/578373002/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/578373002/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:132: enum
AllowCertErrorOptionsMask {
This should probably be in its own file in content/public/browser, since both
content/ and chrome/ code depend on it.

https://codereview.chromium.org/578373002/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:133: OVERRIDABLE = 1 << 0,
From the Content API guidelines:
"enum values should start with the name of the type, i.e. PAGE_TRANSITION_LINK
in the content::PageTransition enum"
http://www.chromium.org/developers/content-module/content-api

(We may want a better enum name in that case, like SSLCertErrorOptions?)

[jam, 2014-09-19 20:04:30]

On 2014/09/19 20:00:12, Charlie Reis wrote:
> The tradeoff here is between "fanning out" the mask into 3 bools in the public
> API vs. introducing a new public enum that both content/ and chrome/ can use. 
I
> think this is a reasonable change, but I do want John's approval before we
land
> it (since he's the guardian of the API).
I have a preference for not introducing new enums unless we need to. that's my
personal preference though, and since Charlie is the reviewer, I defer to him :)

[jww, 2014-09-19 20:07:02]

On 2014/09/19 20:04:30, jam wrote:
> On 2014/09/19 20:00:12, Charlie Reis wrote:
> > The tradeoff here is between "fanning out" the mask into 3 bools in the
public
> > API vs. introducing a new public enum that both content/ and chrome/ can
use. 
> I
> > think this is a reasonable change, but I do want John's approval before we
> land
> > it (since he's the guardian of the API).
> I have a preference for not introducing new enums unless we need to. that's my
> personal preference though, and since Charlie is the reviewer, I defer to him
:)

I'm fine either way, so Charlie, what do you think? I'll wait on addressing your
comments until I get final feedback on your opinion. To state the obvious, the
fundamental problem here can be solved without the content/ API change, so I'm
happy to change it back.

[Charlie Reis, 2014-09-19 20:22:13]

On 2014/09/19 20:07:02, jww wrote:
> On 2014/09/19 20:04:30, jam wrote:
> > On 2014/09/19 20:00:12, Charlie Reis wrote:
> > > The tradeoff here is between "fanning out" the mask into 3 bools in the
> public
> > > API vs. introducing a new public enum that both content/ and chrome/ can
> use. 
> > I
> > > think this is a reasonable change, but I do want John's approval before we
> > land
> > > it (since he's the guardian of the API).
> > I have a preference for not introducing new enums unless we need to. that's
my
> > personal preference though, and since Charlie is the reviewer, I defer to
him
> :)
> 
> I'm fine either way, so Charlie, what do you think? I'll wait on addressing
your
> comments until I get final feedback on your opinion. To state the obvious, the
> fundamental problem here can be solved without the content/ API change, so I'm
> happy to change it back.

Yeah, let's change it back.  While the bug was partly due to having to do an
extra bitmask operation for the translation, I don't see this as an enum that
other places in the code will need to deal with, so it probably doesn't merit a
whole file in the public API.  (If the list of options grows substantially or is
needed in more places, then the tradeoff might be worth it.)

Plus, less work for you.  :)

[jww, 2014-09-19 21:02:34]

On 2014/09/19 20:22:13, Charlie Reis wrote:
> On 2014/09/19 20:07:02, jww wrote:
> > On 2014/09/19 20:04:30, jam wrote:
> > > On 2014/09/19 20:00:12, Charlie Reis wrote:
> > > > The tradeoff here is between "fanning out" the mask into 3 bools in the
> > public
> > > > API vs. introducing a new public enum that both content/ and chrome/ can
> > use. 
> > > I
> > > > think this is a reasonable change, but I do want John's approval before
we
> > > land
> > > > it (since he's the guardian of the API).
> > > I have a preference for not introducing new enums unless we need to.
that's
> my
> > > personal preference though, and since Charlie is the reviewer, I defer to
> him
> > :)
> > 
> > I'm fine either way, so Charlie, what do you think? I'll wait on addressing
> your
> > comments until I get final feedback on your opinion. To state the obvious,
the
> > fundamental problem here can be solved without the content/ API change, so
I'm
> > happy to change it back.
> 
> Yeah, let's change it back.  While the bug was partly due to having to do an
> extra bitmask operation for the translation, I don't see this as an enum that
> other places in the code will need to deal with, so it probably doesn't merit
a
> whole file in the public API.  (If the list of options grows substantially or
is
> needed in more places, then the tradeoff might be worth it.)
> 
> Plus, less work for you.  :)

Agreed all around. And in the end, it should make the merge much simpler as
well. Thanks for taking a look!

[jww, 2014-09-19 21:03:02]

jww@chromium.org changed reviewers:
- creis@chromium.org, jam@chromium.org

[jww, 2014-09-19 21:03:16]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-09-19 21:04:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/578373002/40001

[commit-bot: I haz the power, 2014-09-19 21:04:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/578373002/40001

[Charlie Reis, 2014-09-19 21:09:39]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2014-09-19 21:09:39]

Great.  Not that you need it anymore, but LGTM.

[commit-bot: I haz the power, 2014-09-19 21:54:42]

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

[commit-bot: I haz the power, 2014-09-19 21:54:44]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[jww, 2014-09-19 22:15:43]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-09-19 22:17:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/578373002/40001

[commit-bot: I haz the power, 2014-09-19 23:27:07]

Committed patchset #3 (id:40001) as b800a7efeaf30ee792ee874270ba3b8433a8d1c0

[commit-bot: I haz the power, 2014-09-19 23:28:05]

Patchset 3 (id:??) landed as
https://crrev.com/3a18d0b750ce048eed897ca3e22a3bccaec2fd3c
Cr-Commit-Position: refs/heads/master@{#295807}