Change default value of has_image_contents

Change default value of has_image_contents

The default value of has_image_contents(in context_menu_params.cc, WebContextMenuData.h) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas.
This value is being set properly incase of canvas and image elements
in ContextMenuClientImpl::ShowContextMenu and it is being used only
incase of media_type kMediaTypeImage in render_view_context_menu.cc.
So this change will not cause any difference to the existing code flow.

BUG=707789
Review-Url: https://codereview.chromium.org/2803233002
Cr-Commit-Position: refs/heads/master@{#464717}
Committed: https://chromium.googlesource.com/chromium/src/+/d68c1beaad7929dddc6e6e1278dd03fedcd348eb

[Srirama, 2017-04-07 09:47:02]

The CQ bit was checked by srirama.m@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 09:47:20]

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-04-07 11:20:34]

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

[commit-bot: I haz the power, 2017-04-07 11:20:35]

Dry run: This issue passed the CQ dry run.

[Srirama, 2017-04-07 11:38:50]

Description was changed from

==========
fix

BUG=
==========

to

==========
change default value of hasImageContents and has_image_contents

The default value of hasImageContents(WebContextMenuData.h) and
has_image_contents(context_menu_params.cc) is true bydefault.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas

BUG=707789
==========

[Srirama, 2017-04-07 11:55:58]

srirama.m@samsung.com changed reviewers:
+ avi@chromium.org, foolip@chromium.org, sky@chromium.org

[Srirama, 2017-04-07 11:55:59]

PTAL.

For the record, the original change is
https://chromiumcodereview.appspot.com/23200006/
This change will align with the description here
https://cs.chromium.org/chromium/src/content/public/common/context_menu_param...

[sky, 2017-04-07 14:43:58]

sky@chromium.org changed reviewers:
- sky@chromium.org

[sky, 2017-04-07 14:43:58]

I'm not an owner of any of this code. When in doubt, prefer an OWNER in the
nearest directory. Removing myself as a reviewer.

[foolip, 2017-04-07 15:08:34]

Description was changed from

==========
change default value of hasImageContents and has_image_contents

The default value of hasImageContents(WebContextMenuData.h) and
has_image_contents(context_menu_params.cc) is true bydefault.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas

BUG=707789
==========

to

==========
change default value of hasImageContents and has_image_contents

The default value of hasImageContents(WebContextMenuData.h) and
has_image_contents(context_menu_params.cc) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas

BUG=707789
==========

[foolip, 2017-04-07 15:08:44]

Description was changed from

==========
change default value of hasImageContents and has_image_contents

The default value of hasImageContents(WebContextMenuData.h) and
has_image_contents(context_menu_params.cc) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas

BUG=707789
==========

to

==========
Change default value of hasImageContents and has_image_contents

The default value of hasImageContents(WebContextMenuData.h) and
has_image_contents(context_menu_params.cc) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas

BUG=707789
==========

[foolip, 2017-04-07 15:12:57]

Can you describe how WebContextMenuData::hasImageContents ends up being used,
i.e. why this change does not affect anything or need tests?

[Avi (use Gerrit), 2017-04-07 15:31:53]

On 2017/04/07 15:12:57, foolip_UTC7 wrote:
> Can you describe how WebContextMenuData::hasImageContents ends up being used,
> i.e. why this change does not affect anything or need tests?

I'm a bit ambivalent about this change.

has_image_contents has the meaning "if this is an image, there's data there".
Right? The bug here is that for non-image types, this is true? But it doesn't
apply to non-image types...

[Srirama, 2017-04-08 17:47:28]

On 2017/04/07 15:31:53, Avi (OOO 10-12 April) wrote:
> On 2017/04/07 15:12:57, foolip_UTC7 wrote:
> > Can you describe how WebContextMenuData::hasImageContents ends up being
used,
> > i.e. why this change does not affect anything or need tests?
> 
> I'm a bit ambivalent about this change.
> 
> has_image_contents has the meaning "if this is an image, there's data there".
> Right? The bug here is that for non-image types, this is true? But it doesn't
> apply to non-image types...

@Philipj, @Avi, ideally has_image_contents should be used only along with
media_type image. For other types this variable should not be used and in the
existing chromium code it is being used properly.
The only confusion is the comment given for has_image_contents(This is true if
the context menu was invoked on an image which has non-empty contents) which
gives a meaning that it will be false in all other cases.
I am ok with changing the default value to true or modifying the comment to make
it clear(use this variable only for media_type image) or just keeping the
existing things as it is if you guys think it is fine.
What prompted me to raise this issue is that we are implementing the context
menu for our tizen browser.
In one of the cases we were using has_image_contents variable along with src_url
for displaying image context menu options, without checking for media_type.
So we were getting image context menu options when we long press on video
element.
At first we were bit confused with the comment of the variable
has_image_contents and lead us to thinking that blink engine is not filling this
variable properly. But then after digging a bit deep we understood that this
variable is bydefault true and it should be used only for media_type image.

[Srirama, 2017-04-09 02:40:29]

On 2017/04/08 17:47:28, Srirama wrote:
> On 2017/04/07 15:31:53, Avi (OOO 10-12 April) wrote:
> > On 2017/04/07 15:12:57, foolip_UTC7 wrote:
> > > Can you describe how WebContextMenuData::hasImageContents ends up being
> used,
> > > i.e. why this change does not affect anything or need tests?
> > 
> > I'm a bit ambivalent about this change.
> > 
> > has_image_contents has the meaning "if this is an image, there's data
there".
> > Right? The bug here is that for non-image types, this is true? But it
doesn't
> > apply to non-image types...
> 
> @Philipj, @Avi, ideally has_image_contents should be used only along with
> media_type image. For other types this variable should not be used and in the
> existing chromium code it is being used properly.
> The only confusion is the comment given for has_image_contents(This is true if
> the context menu was invoked on an image which has non-empty contents) which
> gives a meaning that it will be false in all other cases.
> I am ok with changing the default value to true or modifying the comment to
make
Changing default value to **false**
> it clear(use this variable only for media_type image) or just keeping the
> existing things as it is if you guys think it is fine.
> What prompted me to raise this issue is that we are implementing the context
> menu for our tizen browser.
> In one of the cases we were using has_image_contents variable along with
src_url
> for displaying image context menu options, without checking for media_type.
> So we were getting image context menu options when we long press on video
> element.
> At first we were bit confused with the comment of the variable
> has_image_contents and lead us to thinking that blink engine is not filling
this
> variable properly. But then after digging a bit deep we understood that this
> variable is bydefault true and it should be used only for media_type image.

[foolip, 2017-04-10 07:41:32]

On 2017/04/09 02:40:29, Srirama wrote:
> On 2017/04/08 17:47:28, Srirama wrote:
> > On 2017/04/07 15:31:53, Avi (OOO 10-12 April) wrote:
> > > On 2017/04/07 15:12:57, foolip_UTC7 wrote:
> > > > Can you describe how WebContextMenuData::hasImageContents ends up being
> > used,
> > > > i.e. why this change does not affect anything or need tests?
> > > 
> > > I'm a bit ambivalent about this change.
> > > 
> > > has_image_contents has the meaning "if this is an image, there's data
> there".
> > > Right? The bug here is that for non-image types, this is true? But it
> doesn't
> > > apply to non-image types...
> > 
> > @Philipj, @Avi, ideally has_image_contents should be used only along with
> > media_type image. For other types this variable should not be used and in
the
> > existing chromium code it is being used properly.
> > The only confusion is the comment given for has_image_contents(This is true
if
> > the context menu was invoked on an image which has non-empty contents) which
> > gives a meaning that it will be false in all other cases.
> > I am ok with changing the default value to true or modifying the comment to
> make
> Changing default value to **false**
> > it clear(use this variable only for media_type image) or just keeping the
> > existing things as it is if you guys think it is fine.
> > What prompted me to raise this issue is that we are implementing the context
> > menu for our tizen browser.
> > In one of the cases we were using has_image_contents variable along with
> src_url
> > for displaying image context menu options, without checking for media_type.
> > So we were getting image context menu options when we long press on video
> > element.
> > At first we were bit confused with the comment of the variable
> > has_image_contents and lead us to thinking that blink engine is not filling
> this
> > variable properly. But then after digging a bit deep we understood that this
> > variable is bydefault true and it should be used only for media_type image.

At the very least the documentation should be fixed, but if you can describe how
WebContextMenuData::hasImageContents ends up being used and why this change
doesn't cause a different code path to be taken at any point, that'd be nicer I
think.

[Srirama, 2017-04-10 15:09:30]

On 2017/04/10 07:41:32, foolip_UTC7 wrote:
> On 2017/04/09 02:40:29, Srirama wrote:
> > On 2017/04/08 17:47:28, Srirama wrote:
> > > On 2017/04/07 15:31:53, Avi (OOO 10-12 April) wrote:
> > > > On 2017/04/07 15:12:57, foolip_UTC7 wrote:
> > > > > Can you describe how WebContextMenuData::hasImageContents ends up
being
> > > used,
> > > > > i.e. why this change does not affect anything or need tests?
> > > > 
> > > > I'm a bit ambivalent about this change.
> > > > 
> > > > has_image_contents has the meaning "if this is an image, there's data
> > there".
> > > > Right? The bug here is that for non-image types, this is true? But it
> > doesn't
> > > > apply to non-image types...
> > > 
> > > @Philipj, @Avi, ideally has_image_contents should be used only along with
> > > media_type image. For other types this variable should not be used and in
> the
> > > existing chromium code it is being used properly.
> > > The only confusion is the comment given for has_image_contents(This is
true
> if
> > > the context menu was invoked on an image which has non-empty contents)
which
> > > gives a meaning that it will be false in all other cases.
> > > I am ok with changing the default value to true or modifying the comment
to
> > make
> > Changing default value to **false**
> > > it clear(use this variable only for media_type image) or just keeping the
> > > existing things as it is if you guys think it is fine.
> > > What prompted me to raise this issue is that we are implementing the
context
> > > menu for our tizen browser.
> > > In one of the cases we were using has_image_contents variable along with
> > src_url
> > > for displaying image context menu options, without checking for
media_type.
> > > So we were getting image context menu options when we long press on video
> > > element.
> > > At first we were bit confused with the comment of the variable
> > > has_image_contents and lead us to thinking that blink engine is not
filling
> > this
> > > variable properly. But then after digging a bit deep we understood that
this
> > > variable is bydefault true and it should be used only for media_type
image.
> 
> At the very least the documentation should be fixed, but if you can describe
how
> WebContextMenuData::hasImageContents ends up being used and why this change
> doesn't cause a different code path to be taken at any point, that'd be nicer
I
> think.

Firstly it is getting set only incase of canvas and image nodes. If it is canvas
node it is set to true again. If it is image and there is no error in image
load/decode then it is set to true and if the image has some error then it is
set to false.
Now this is getting used in render_view_context_menu.cc to build the context
menu options.
Here the context menu options are added based on the selected node. The image
context menu options (for ex: save image, copy image) are added only if the
current media_type is image and the variable has_image_contents is true.
So before adding image context menu options, media_type is being checked
properly (ContextMenuContentType::SupportsGroupInternal)

[Srirama, 2017-04-11 06:06:32]

On 2017/04/10 15:09:30, Srirama wrote:
> On 2017/04/10 07:41:32, foolip_UTC7 wrote:
> > On 2017/04/09 02:40:29, Srirama wrote:
> > > On 2017/04/08 17:47:28, Srirama wrote:
> > > > On 2017/04/07 15:31:53, Avi (OOO 10-12 April) wrote:
> > > > > On 2017/04/07 15:12:57, foolip_UTC7 wrote:
> > > > > > Can you describe how WebContextMenuData::hasImageContents ends up
> being
> > > > used,
> > > > > > i.e. why this change does not affect anything or need tests?
> > > > > 
> > > > > I'm a bit ambivalent about this change.
> > > > > 
> > > > > has_image_contents has the meaning "if this is an image, there's data
> > > there".
> > > > > Right? The bug here is that for non-image types, this is true? But it
> > > doesn't
> > > > > apply to non-image types...
> > > > 
> > > > @Philipj, @Avi, ideally has_image_contents should be used only along
with
> > > > media_type image. For other types this variable should not be used and
in
> > the
> > > > existing chromium code it is being used properly.
> > > > The only confusion is the comment given for has_image_contents(This is
> true
> > if
> > > > the context menu was invoked on an image which has non-empty contents)
> which
> > > > gives a meaning that it will be false in all other cases.
> > > > I am ok with changing the default value to true or modifying the comment
> to
> > > make
> > > Changing default value to **false**
> > > > it clear(use this variable only for media_type image) or just keeping
the
> > > > existing things as it is if you guys think it is fine.
> > > > What prompted me to raise this issue is that we are implementing the
> context
> > > > menu for our tizen browser.
> > > > In one of the cases we were using has_image_contents variable along with
> > > src_url
> > > > for displaying image context menu options, without checking for
> media_type.
> > > > So we were getting image context menu options when we long press on
video
> > > > element.
> > > > At first we were bit confused with the comment of the variable
> > > > has_image_contents and lead us to thinking that blink engine is not
> filling
> > > this
> > > > variable properly. But then after digging a bit deep we understood that
> this
> > > > variable is bydefault true and it should be used only for media_type
> image.
> > 
> > At the very least the documentation should be fixed, but if you can describe
> how
> > WebContextMenuData::hasImageContents ends up being used and why this change
> > doesn't cause a different code path to be taken at any point, that'd be
nicer
> I
> > think.
> 
> Firstly it is getting set only incase of canvas and image nodes. If it is
canvas
> node it is set to true again. If it is image and there is no error in image
> load/decode then it is set to true and if the image has some error then it is
> set to false.
> Now this is getting used in render_view_context_menu.cc to build the context
> menu options.
> Here the context menu options are added based on the selected node. The image
> context menu options (for ex: save image, copy image) are added only if the
> current media_type is image and the variable has_image_contents is true.
> So before adding image context menu options, media_type is being checked
> properly (ContextMenuContentType::SupportsGroupInternal)

This change doesn't cause a different code flow because has_image_content should
be used only with media_type image and canvas only and this variable is being
set properly in these two cases in ContextMenuClientImpl::ShowContextMenu
function.

[foolip, 2017-04-11 14:12:01]

On 2017/04/11 06:06:32, Srirama wrote:
> On 2017/04/10 15:09:30, Srirama wrote:
> > On 2017/04/10 07:41:32, foolip_UTC7 wrote:
> > > On 2017/04/09 02:40:29, Srirama wrote:
> > > > On 2017/04/08 17:47:28, Srirama wrote:
> > > > > On 2017/04/07 15:31:53, Avi (OOO 10-12 April) wrote:
> > > > > > On 2017/04/07 15:12:57, foolip_UTC7 wrote:
> > > > > > > Can you describe how WebContextMenuData::hasImageContents ends up
> > being
> > > > > used,
> > > > > > > i.e. why this change does not affect anything or need tests?
> > > > > > 
> > > > > > I'm a bit ambivalent about this change.
> > > > > > 
> > > > > > has_image_contents has the meaning "if this is an image, there's
data
> > > > there".
> > > > > > Right? The bug here is that for non-image types, this is true? But
it
> > > > doesn't
> > > > > > apply to non-image types...
> > > > > 
> > > > > @Philipj, @Avi, ideally has_image_contents should be used only along
> with
> > > > > media_type image. For other types this variable should not be used and
> in
> > > the
> > > > > existing chromium code it is being used properly.
> > > > > The only confusion is the comment given for has_image_contents(This is
> > true
> > > if
> > > > > the context menu was invoked on an image which has non-empty contents)
> > which
> > > > > gives a meaning that it will be false in all other cases.
> > > > > I am ok with changing the default value to true or modifying the
comment
> > to
> > > > make
> > > > Changing default value to **false**
> > > > > it clear(use this variable only for media_type image) or just keeping
> the
> > > > > existing things as it is if you guys think it is fine.
> > > > > What prompted me to raise this issue is that we are implementing the
> > context
> > > > > menu for our tizen browser.
> > > > > In one of the cases we were using has_image_contents variable along
with
> > > > src_url
> > > > > for displaying image context menu options, without checking for
> > media_type.
> > > > > So we were getting image context menu options when we long press on
> video
> > > > > element.
> > > > > At first we were bit confused with the comment of the variable
> > > > > has_image_contents and lead us to thinking that blink engine is not
> > filling
> > > > this
> > > > > variable properly. But then after digging a bit deep we understood
that
> > this
> > > > > variable is bydefault true and it should be used only for media_type
> > image.
> > > 
> > > At the very least the documentation should be fixed, but if you can
describe
> > how
> > > WebContextMenuData::hasImageContents ends up being used and why this
change
> > > doesn't cause a different code path to be taken at any point, that'd be
> nicer
> > I
> > > think.
> > 
> > Firstly it is getting set only incase of canvas and image nodes. If it is
> canvas
> > node it is set to true again. If it is image and there is no error in image
> > load/decode then it is set to true and if the image has some error then it
is
> > set to false.
> > Now this is getting used in render_view_context_menu.cc to build the context
> > menu options.
> > Here the context menu options are added based on the selected node. The
image
> > context menu options (for ex: save image, copy image) are added only if the
> > current media_type is image and the variable has_image_contents is true.
> > So before adding image context menu options, media_type is being checked
> > properly (ContextMenuContentType::SupportsGroupInternal)
> 
> This change doesn't cause a different code flow because has_image_content
should
> be used only with media_type image and canvas only and this variable is being
> set properly in these two cases in ContextMenuClientImpl::ShowContextMenu
> function.

Thanks, LGTM with an explanation like that in the commit message.

avi@, are you still skeptical?

[Avi (use Gerrit), 2017-04-13 14:58:36]

LGTM

This is OK.

[Srirama, 2017-04-14 07:09:18]

Description was changed from

==========
Change default value of hasImageContents and has_image_contents

The default value of hasImageContents(WebContextMenuData.h) and
has_image_contents(context_menu_params.cc) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas

BUG=707789
==========

to

==========
Change default value of hasImageContents and has_image_contents

The default value of hasImageContents(WebContextMenuData.h) and
has_image_contents(context_menu_params.cc) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas.
This value is being set properly incase of canvas and image elements
in ContextMenuClientImpl::ShowContextMenu and it is being used only
incase of media_type kMediaTypeImage in render_view_context_menu.cc.

BUG=707789
==========

[Srirama, 2017-04-14 07:10:49]

Description was changed from

==========
Change default value of hasImageContents and has_image_contents

The default value of hasImageContents(WebContextMenuData.h) and
has_image_contents(context_menu_params.cc) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas.
This value is being set properly incase of canvas and image elements
in ContextMenuClientImpl::ShowContextMenu and it is being used only
incase of media_type kMediaTypeImage in render_view_context_menu.cc.

BUG=707789
==========

to

==========
Change default value of hasImageContents and has_image_contents

The default value of hasImageContents(WebContextMenuData.h) and
has_image_contents(context_menu_params.cc) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas.
This value is being set properly incase of canvas and image elements
in ContextMenuClientImpl::ShowContextMenu and it is being used only
incase of media_type kMediaTypeImage in render_view_context_menu.cc.
So this change will not cause any difference to the existing code flow.

BUG=707789
==========

[Srirama, 2017-04-14 07:13:07]

Description was changed from

==========
Change default value of hasImageContents and has_image_contents

The default value of hasImageContents(WebContextMenuData.h) and
has_image_contents(context_menu_params.cc) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas.
This value is being set properly incase of canvas and image elements
in ContextMenuClientImpl::ShowContextMenu and it is being used only
incase of media_type kMediaTypeImage in render_view_context_menu.cc.
So this change will not cause any difference to the existing code flow.

BUG=707789
==========

to

==========
Change default value of has_image_contents

The default value of has_image_contents(in context_menu_params.cc,
WebContextMenuData.h) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas.
This value is being set properly incase of canvas and image elements
in ContextMenuClientImpl::ShowContextMenu and it is being used only
incase of media_type kMediaTypeImage in render_view_context_menu.cc.
So this change will not cause any difference to the existing code flow.

BUG=707789
==========

[Srirama, 2017-04-14 10:34:23]

The CQ bit was checked by srirama.m@samsung.com

[Srirama, 2017-04-14 10:34:24]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2803233002/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-14 10:34:39]

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-04-14 12:04:32]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492166063839060,
"parent_rev": "dbd6183c431953b0e5f6513f2d3b48bd4a097d3a", "commit_rev":
"d68c1beaad7929dddc6e6e1278dd03fedcd348eb"}

[commit-bot: I haz the power, 2017-04-14 12:05:16]

Description was changed from

==========
Change default value of has_image_contents

The default value of has_image_contents(in context_menu_params.cc,
WebContextMenuData.h) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas.
This value is being set properly incase of canvas and image elements
in ContextMenuClientImpl::ShowContextMenu and it is being used only
incase of media_type kMediaTypeImage in render_view_context_menu.cc.
So this change will not cause any difference to the existing code flow.

BUG=707789
==========

to

==========
Change default value of has_image_contents

The default value of has_image_contents(in context_menu_params.cc,
WebContextMenuData.h) is true by default.
These values mean there is an image content for the long pressed node.

Changed default value to false to make it less confusing for
elements(Link, video) other than image, canvas.
This value is being set properly incase of canvas and image elements
in ContextMenuClientImpl::ShowContextMenu and it is being used only
incase of media_type kMediaTypeImage in render_view_context_menu.cc.
So this change will not cause any difference to the existing code flow.

BUG=707789

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

[commit-bot: I haz the power, 2017-04-14 12:05:18]

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