1287657 - Context menu items should not be disabled for media with a blob URL

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect, P2)

Description

[Geoff Lankow (:darktrojan)]

Bug 1094310 disabled some context menu items on video/audio with blob URLs. If the URL hasn't been revoked, these should still be available:

context-savevideo
context-saveaudio
context-viewvideo

Maybe these:

context-copyvideourl
context-copyaudiourl

But not these:

context-sendvideo
context-sendaudio
context-sharevideo

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Do you know how to tell if it has been revoked or not?

Comment 2

[Geoff Lankow (:darktrojan)]

(In reply to (unavailable 07/20-07/21) Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Do you know how to tell if it has been revoked or not?

I don't. I'm not sure it's even possible. Andrea?

Comment 3

[Andrea Marchesini [:baku]]

> I don't. I'm not sure it's even possible. Andrea?

Not yet. But it's easy to implement. What I suggest is this:

Add a [chromeOnly] method in URL API such as:

// mozilla only
partial interface URL {
  [ChromeOnly]
  static bool isValidURL(DOMString URL);
}

This method should call a new method in nsHostObjectProtocolHandler: static bool HasDataEntry(const nsACString& aURL) where you just call GetDataInfo().

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Geoff, would you like to implement this?

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1287657-1.diff

Matthew, since you made the original change, please review the browser bits.

Andrea, please review the DOM bits, and please bear in mind that I don't really know what I'm doing here. I've made it work and I think I've done it right, but...

Comment 6

[Matthew Gregan [:kinetik]]

Comment on attachment 8774050 [details] [diff] [review]
1287657-1.diff

LGTM

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8774050 [details] [diff] [review]
1287657-1.diff

Review of attachment 8774050 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/nsContextMenu.js
@@ +1649,5 @@
>    isMediaURLReusable: function(aURL) {
> +    if (aURL.startsWith("blob:")) {
> +      return URL.isValidURL(aURL);
> +    }
> +    return !aURL.startsWith("mediasource:");

we don't have this mediasource: URL anymore.

::: dom/base/nsHostObjectProtocolHandler.cpp
@@ +511,5 @@
>    delete gDataTable;
>    gDataTable = nullptr;
>  }
>  
> +bool

/* static */ bool

@@ +512,5 @@
>    gDataTable = nullptr;
>  }
>  
> +bool
> +nsHostObjectProtocolHandler::HasDataEntry(const nsACString& aUri) {

{ in a new line

@@ +513,5 @@
>  }
>  
> +bool
> +nsHostObjectProtocolHandler::HasDataEntry(const nsACString& aUri) {
> +  return GetDataInfo(aUri);

return !!GetDataInfo(aUri);

::: dom/url/URL.cpp
@@ +1381,5 @@
> +    new IsValidURLRunnable(workerPrivate, aUrl, &value);
> +
> +  runnable->Dispatch(aRv);
> +  NS_WARN_IF(aRv.Failed());
> +  return value;

Instead passing this boolean var to the runnable, what about if you do:

return runnable->IsvalidURL();

and in the runnable, you store the boolean value internally.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1287657-2.diff

I know you've already reviewed this, but it's better to check than to stuff it up and have to fix it.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8774275 [details] [diff] [review]
1287657-2.diff

Review of attachment 8774275 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/url/URL.cpp
@@ +941,5 @@
> +  IsValidURLRunnable(WorkerPrivate* aWorkerPrivate,
> +                     const nsAString& aURL)
> +  : WorkerMainThreadRunnable(aWorkerPrivate,
> +                             NS_LITERAL_CSTRING("URL :: IsValidURL"))
> +  , mURL(aURL)

, mValid(false)

@@ +956,5 @@
> +    return true;
> +  }
> +
> +  bool
> +  IsValidURL()

const

@@ +1383,5 @@
> +  RefPtr<IsValidURLRunnable> runnable =
> +    new IsValidURLRunnable(workerPrivate, aUrl);
> +
> +  runnable->Dispatch(aRv);
> +  NS_WARN_IF(aRv.Failed());

if (NS_WARN_IF(aRv.Failed())) {
  return false;
}

return runnable->IsValidURL();

@@ +1785,5 @@
> +                ErrorResult& aRv)
> +{
> +  if (NS_IsMainThread()) {
> +    return URLMainThread::IsValidURL(aGlobal, aURL, aRv);
> +  } else {

if (NS_IsMainThread()) {
  return URLMainThread::IsValidURL(...);
}

return URLWorker::IsValidURL(...);

Comment 10

[Matthew Gregan [:kinetik]]

(In reply to Andrea Marchesini (:baku) from comment #7)
> Comment on attachment 8774050 [details] [diff] [review]
> 1287657-1.diff
> 
> Review of attachment 8774050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/nsContextMenu.js
> @@ +1649,5 @@
> >    isMediaURLReusable: function(aURL) {
> > +    if (aURL.startsWith("blob:")) {
> > +      return URL.isValidURL(aURL);
> > +    }
> > +    return !aURL.startsWith("mediasource:");
> 
> we don't have this mediasource: URL anymore.

I'm confused what you mean here... in the JS console:

> URL.createObjectURL(new MediaSource())
<< "mediasource:https://bugzilla.mozilla.org/7967be8b-12da-4919-a24f-aedccd4b2656"

Comment 11

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #10)
> I'm confused what you mean here... in the JS console:
> 
> > URL.createObjectURL(new MediaSource())
> <<
> "mediasource:https://bugzilla.mozilla.org/7967be8b-12da-4919-a24f-
> aedccd4b2656"

Nevermind, this just changed recently enough that my Nightly and DXR weren't showing the change yet.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce7d3f03926
Context menu items should not be disabled for media with a blob URL; r=kinetik, r=baku

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ce7d3f03926