Validate windows.create API's state input parameter.

Validate windows.create API's state input parameters those cannot live together.

These conditions are coming from windows.update API.
- minimized with 'focused'.
- maximized, fullscreen with 'not-focused'
- Having bounds with minimized, maximized, fullscreen state.
- panel type with minimized, maximized, fullscreen state.

BUG=459841
TEST=browser_tests --gtest_filter=ExtensionWindowCreateTest.*

Committed: https://crrev.com/aa246b7c7da6f673569616c46e5a5f27f8639817
Cr-Commit-Position: refs/heads/master@{#326710}

[limasdf, 2015-04-20 09:27:33]

limasdf@gmail.com changed reviewers:
+ kalman@chromium.org

[limasdf, 2015-04-20 09:31:00]

Kalman, Please take a look.

[not at google - send to devlin, 2015-04-20 16:32:55]

https://codereview.chromium.org/1055933009/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1055933009/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_api.cc:218: }
Let's try to make this as compact as possible, it's a lot of code. I *think* it
would be easier to read if you inverted the conditions and used a switch
instead?

bool focused = create_data->focused && *create_data->focused;
bool has_bound = create_data...;
bool is_panel = ...;
switch (state) {
  case MINIMIZED:
    return !focused && !has_bound && ...;
  case MAXIMIZED:
    return focused && !has_bound && ...;
  case ...l
}

https://codereview.chromium.org/1055933009/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/1055933009/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_test.cc:59:
scoped_refptr<WindowsCreateFunction> function_;
Can you make these tests sateless? I don't know why you need to store these as
member variables.

[limasdf, 2015-04-22 15:44:46]

Patchset #2 (id:20001) has been deleted

[limasdf, 2015-04-22 15:49:33]

Sorry for the late response.

https://codereview.chromium.org/1055933009/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1055933009/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_api.cc:218: }
On 2015/04/20 16:32:55, kalman wrote:
> Let's try to make this as compact as possible, it's a lot of code. I *think*
it
> would be easier to read if you inverted the conditions and used a switch
> instead?
> 
> bool focused = create_data->focused && *create_data->focused;
> bool has_bound = create_data...;
> bool is_panel = ...;
> switch (state) {
>   case MINIMIZED:
>     return !focused && !has_bound && ...;
>   case MAXIMIZED:
>     return focused && !has_bound && ...;
>   case ...l
> }

Great idea. Done.

https://codereview.chromium.org/1055933009/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/1055933009/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_test.cc:59:
scoped_refptr<WindowsCreateFunction> function_;
On 2015/04/20 16:32:55, kalman wrote:
> Can you make these tests sateless? I don't know why you need to store these as
> member variables.

You're exactly right. Done.

[not at google - send to devlin, 2015-04-22 17:46:32]

Looks great, one last question.

https://codereview.chromium.org/1055933009/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1055933009/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:218: // If 'focused' is not set,
then treat it as focused true.
So is focused default true, or default false? or does it depend on the initial
state?

i.e. should the assignment of |focused| be (!create_data->focused ||
*create_data->focused)?

(I don't know the answer)

[limasdf, 2015-04-23 14:45:33]

https://codereview.chromium.org/1055933009/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1055933009/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:218: // If 'focused' is not set,
then treat it as focused true.
On 2015/04/22 17:46:32, kalman wrote:
> So is focused default true, or default false? or does it depend on the initial
> state?
> 
> i.e. should the assignment of |focused| be (!create_data->focused ||
> *create_data->focused)?
> 
> (I don't know the answer)

It depends on initial state.  if |focused| is (!create_data->focused ||
*create_data->focused), 
it fails in {state:minimized} without focused value. (|focused| become true,
which is wrong).

There's no single variable to determine(i.e. |!focused| for minimized state,
|focused| for other states). Here's proof, http://goo.gl/5AZhZR

[not at google - send to devlin, 2015-04-23 20:58:52]

lgtm with that last suggested change

https://codereview.chromium.org/1055933009/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1055933009/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:218: // If 'focused' is not set,
then treat it as focused true.
On 2015/04/23 14:45:32, Sung-guk Lim wrote:
> On 2015/04/22 17:46:32, kalman wrote:
> > So is focused default true, or default false? or does it depend on the
initial
> > state?
> > 
> > i.e. should the assignment of |focused| be (!create_data->focused ||
> > *create_data->focused)?
> > 
> > (I don't know the answer)
> 
> It depends on initial state.  if |focused| is (!create_data->focused ||
> *create_data->focused), 
> it fails in {state:minimized} without focused value. (|focused| become true,
> which is wrong).
> 
> There's no single variable to determine(i.e. |!focused| for minimized state,
> |focused| for other states). Here's proof, http://goo.gl/5AZhZR

ah :) in that case having a |focused| variable is a bit confusing, you might as
well write it like:

case...
// If minimised, default focused state should be unfocused.
return !(create_data->focused && *create_data->focused) && !has_bound &&
!is_panel;
case...
// If maximised/fullscreen, default focused state should be focused.
return !(create_data->focused && !*create_data->focused)

[limasdf, 2015-04-23 23:51:42]

I'm happy that you understand my bad English!

Thanks for the review.

https://codereview.chromium.org/1055933009/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1055933009/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:218: // If 'focused' is not set,
then treat it as focused true.
On 2015/04/23 20:58:52, kalman wrote:
> On 2015/04/23 14:45:32, Sung-guk Lim wrote:
> > On 2015/04/22 17:46:32, kalman wrote:
> > > So is focused default true, or default false? or does it depend on the
> initial
> > > state?
> > > 
> > > i.e. should the assignment of |focused| be (!create_data->focused ||
> > > *create_data->focused)?
> > > 
> > > (I don't know the answer)
> > 
> > It depends on initial state.  if |focused| is (!create_data->focused ||
> > *create_data->focused), 
> > it fails in {state:minimized} without focused value. (|focused| become true,
> > which is wrong).
> > 
> > There's no single variable to determine(i.e. |!focused| for minimized state,
> > |focused| for other states). Here's proof, http://goo.gl/5AZhZR
> 
> ah :) in that case having a |focused| variable is a bit confusing, you might
as
> well write it like:
> 
> case...
> // If minimised, default focused state should be unfocused.
> return !(create_data->focused && *create_data->focused) && !has_bound &&
> !is_panel;
> case...
> // If maximised/fullscreen, default focused state should be focused.
> return !(create_data->focused && !*create_data->focused)

Done.

[limasdf, 2015-04-23 23:54:01]

The CQ bit was checked by limasdf@gmail.com

[limasdf, 2015-04-23 23:54:02]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1055933009/#ps60001
(title: "focus")

[commit-bot: I haz the power, 2015-04-23 23:55:04]

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

[commit-bot: I haz the power, 2015-04-24 01:13:47]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-04-24 01:15:25]

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