Closed Bug 1287657 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

Attachments

(1 file, 1 obsolete file)

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
Do you know how to tell if it has been revoked or not?
Flags: needinfo?(geoff)
Priority: -- → P2
(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?
Flags: needinfo?(geoff) → needinfo?(amarchesini)
> 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().
Flags: needinfo?(amarchesini)
Geoff, would you like to implement this?
Flags: needinfo?(geoff)
Attached patch 1287657-1.diff (obsolete) — Splinter Review
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...
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Flags: needinfo?(geoff)
Attachment #8774050 - Flags: review?(kinetik)
Attachment #8774050 - Flags: review?(amarchesini)
Comment on attachment 8774050 [details] [diff] [review]
1287657-1.diff

LGTM
Attachment #8774050 - Flags: review?(kinetik) → review+
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.
Attachment #8774050 - Flags: review?(amarchesini) → review+
Attached patch 1287657-2.diffSplinter Review
I know you've already reviewed this, but it's better to check than to stuff it up and have to fix it.
Attachment #8774050 - Attachment is obsolete: true
Attachment #8774275 - Flags: review?(amarchesini)
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(...);
Attachment #8774275 - Flags: review?(amarchesini) → review+
(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"
(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.
Pushed by geoff@darktrojan.net:
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
https://hg.mozilla.org/mozilla-central/rev/2ce7d3f03926
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.