Context menu should not shows data: in heading

Context menu should not shows data: in heading

In some senario image source url has full image data starts with
"data:" so context menu in that case returns source url starts with
"data:" and full contents.In those senario we should show only link url
instead of source url.

BUG=395511
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285880

[Jitu( very slow this week), 2014-07-21 06:03:29]

PTAL

[Bernhard Bauer, 2014-07-21 10:19:28]

https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(right):

https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:42:
if (!params.getSrcUrl().startsWith("data:")) {
Hm, we probably shouldn't give the source URL precedence anyway. If there is a
link target, it's usually more interesting than the image source.

If we would give the link URL precedence (i.e. just swap the order of the two
if-statements), that would also solve this use case.

[Jitu( very slow this week), 2014-07-21 11:11:35]

@Bauer,

PTAL...

https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(right):

https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:42:
if (!params.getSrcUrl().startsWith("data:")) {
Yes you are right... this will solve the problem.

But in some scenario the link URL may be an empty string for image contents. In
that case it will show "about:blank".

So to avoid that i have added like this.

Please let me know your comments in this.

[Bernhard Bauer, 2014-07-21 13:53:39]

https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(right):

https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:42:
if (!params.getSrcUrl().startsWith("data:")) {
On 2014/07/21 11:11:35, Jitu wrote:
> Yes you are right... this will solve the problem.
> 
> But in some scenario the link URL may be an empty string for image contents.
In
> that case it will show "about:blank".

Wait, are you sure you are not confusing the link text with the link URL? In
this situation:

<a href="http://example.com"><img src="http://example.com/image.png"></a>

The link *text* would be empty (because the contents of the link are just an
image), but the link *URL* would be http://example.com.

In the m.imgur.com example, about:blank is the link URL[*], but that has nothing
to do with the fact that the link text is empty, or that the link contains an
image.

Another way to think about it: What you are proposing in this CL would actually
show about:blank again if the image is a data URL (and the link target is
about:blank). That seems to contradict your assumption that we don't want to
show about:blank.

[*] Actually, it seems that the link URL is "javascript:;". I don't really know
why that becomes about:blank. If you want to investigate why, that would be
helpful (and would also get rid of the about:blank in the context menu!)

> So to avoid that i have added like this.
> 
> Please let me know your comments in this.

[Jitu( very slow this week), 2014-07-23 08:21:47]

Added comments please check

https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(right):

https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:42:
if (!params.getSrcUrl().startsWith("data:")) {
Yes you are right..
Link URL is "javascript:" 

This become changed to about:blank while in this Below place while calling
FilterURL()

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

And this is the exart place where it override to about:blank

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

Please let me know on this.

[Bernhard Bauer, 2014-07-23 13:52:04]

On 2014/07/23 08:21:47, Jitu wrote:
> Added comments please check
> 
>
https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
> (right):
> 
>
https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
>
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:42:
> if (!params.getSrcUrl().startsWith("data:")) {
> Yes you are right..
> Link URL is "javascript:" 
> 
> This become changed to about:blank while in this Below place while calling
> FilterURL()
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> 
> And this is the exart place where it override to about:blank
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> Please let me know on this.

Thanks for the investigation! Okay, it seems this is working as intended... So,
I guess we could show the link URL, provided that it's not about:blank. If it
is, what should we do though? We could show the image source URL, but as
mentioned in this bug, that might be a data: URL, and we wouldn't want to show
that either.

Another idea would be to always show the unfiltered URL. That would still be
javascript:, so people would at least see what the link target actually is.
WDYT?

[Jitu( very slow this week), 2014-07-23 14:25:10]

On 2014/07/23 13:52:04, Bernhard Bauer wrote:
> On 2014/07/23 08:21:47, Jitu wrote:
> > Added comments please check
> > 
> >
>
https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:42:
> > if (!params.getSrcUrl().startsWith("data:")) {
> > Yes you are right..
> > Link URL is "javascript:" 
> > 
> > This become changed to about:blank while in this Below place while calling
> > FilterURL()
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > 
> > And this is the exart place where it override to about:blank
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> > 
> > Please let me know on this.
> 
> Thanks for the investigation! Okay, it seems this is working as intended...
So,
> I guess we could show the link URL, provided that it's not about:blank. If it
> is, what should we do though? We could show the image source URL, but as
> mentioned in this bug, that might be a data: URL, and we wouldn't want to show
> that either.
> 
> Another idea would be to always show the unfiltered URL. That would still be
> javascript:, so people would at least see what the link target actually is.
> WDYT?

I was just checking behavior in some other browser,
Some browser does not shows the heading (opera, UC etc)  :).
Some browser like Sleipnir shows "javascript:"
Mozilla shows link URL.

I think it will be good if we can show Link URL or No headers.

[Jitu( very slow this week), 2014-07-24 05:14:24]

@Bauer,

Please let me know which one we can show.

Thanks

[Bernhard Bauer, 2014-07-24 05:31:37]

On 2014/07/23 14:25:10, Jitu wrote:
> On 2014/07/23 13:52:04, Bernhard Bauer wrote:
> > On 2014/07/23 08:21:47, Jitu wrote:
> > > Added comments please check
> > > 
> > >
> >
>
https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
> > > File
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/398893006/diff/1/chrome/android/java/src/org/...
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:42:
> > > if (!params.getSrcUrl().startsWith("data:")) {
> > > Yes you are right..
> > > Link URL is "javascript:" 
> > > 
> > > This become changed to about:blank while in this Below place while calling
> > > FilterURL()
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > > 
> > > And this is the exart place where it override to about:blank
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> > > 
> > > Please let me know on this.
> > 
> > Thanks for the investigation! Okay, it seems this is working as intended...
> So,
> > I guess we could show the link URL, provided that it's not about:blank. If
it
> > is, what should we do though? We could show the image source URL, but as
> > mentioned in this bug, that might be a data: URL, and we wouldn't want to
show
> > that either.
> > 
> > Another idea would be to always show the unfiltered URL. That would still be
> > javascript:, so people would at least see what the link target actually is.
> > WDYT?
> 
> I was just checking behavior in some other browser,
> Some browser does not shows the heading (opera, UC etc)  :).
> Some browser like Sleipnir shows "javascript:"
> Mozilla shows link URL.
> 
> I think it will be good if we can show Link URL or No headers.

Sounds good.

[Jitu( very slow this week), 2014-07-24 05:35:12]

If we will show Link URL then we can push this patch.

Else for No header i can probably send another patch.

Please let me know which way we can go ?

[Jitu( very slow this week), 2014-07-25 03:04:11]

@Bauer,

Can you plz comment on this.

[Bernhard Bauer, 2014-07-25 04:35:01]

On 2014/07/24 05:35:12, Jitu wrote:
> If we will show Link URL then we can push this patch.
> 
> Else for No header i can probably send another patch.
> 
> Please let me know which way we can go ?

I'm sorry, there seems to have been a misunderstanding. This patch will still
show the image URL. What I thought you were saying (and what I agreed to) was to
show the link URL or no header.

[Jitu( very slow this week), 2014-07-25 08:22:19]

Removed the header.

PTAL...

[Bernhard Bauer, 2014-07-25 18:37:02]

On 2014/07/25 08:22:19, Jitu wrote:
> Removed the header.
> 
> PTAL...

And now we show nothing at all. Please stick to what we agreed on, which was the
following: Either show the link URL (filtered or unfiltered), or if it's empty,
show nothing at all. It's absolutely okay if you feel we should do something
different, but you need to bring that up in discussion. I really don't know how
to communicate this clearer.

[Jitu( very slow this week), 2014-07-28 06:14:16]

Changed as per discussion.

If the LinkUrl is blank ( about:blank) then should not show any header.
Else we will show LinkUrl.

PTAL

[Bernhard Bauer, 2014-07-28 07:51:18]

Thanks!

https://codereview.chromium.org/398893006/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(right):

https://codereview.chromium.org/398893006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:42:
if (!TextUtils.isEmpty(params.getLinkUrl()) &&
!params.getLinkUrl().startsWith(BLANK_URL))
params.getLinkUrl().equals(BLANK_URL)?

[Jitu( very slow this week), 2014-07-28 08:01:49]

Changed as per suggested.
PTAL...

Thanks

https://codereview.chromium.org/398893006/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java
(right):

https://codereview.chromium.org/398893006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:42:
if (!TextUtils.isEmpty(params.getLinkUrl()) &&
!params.getLinkUrl().startsWith(BLANK_URL))
On 2014/07/28 07:51:18, Bernhard Bauer wrote:
> params.getLinkUrl().equals(BLANK_URL)?

Done.

[Bernhard Bauer, 2014-07-28 08:08:32]

LGTM!

[Jitu( very slow this week), 2014-07-28 08:09:05]

The CQ bit was checked by jitendra.ks@samsung.com

[commit-bot: I haz the power, 2014-07-28 08:09:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jitendra.ks@samsung.com/398893006/60001

[commit-bot: I haz the power, 2014-07-28 09:22:07]

Change committed as 285880