Closed Bug 1268047 Opened 3 years ago Closed 3 years ago

Change the handling of javascript: return values to align with other browsers and the updated spec

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED INVALID
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Whiteboard: btpp-active)

Attachments

(3 files, 1 obsolete file)

See https://github.com/whatwg/html/pull/1107

The relevant thing is that instead of doing ToString() on the return value we should just ignore it if it's not a string already.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8746156 - Attachment is obsolete: true
Attachment #8746156 - Flags: review?(peterv)
Whiteboard: btpp-active
Comment on attachment 8746155 [details] [diff] [review]
part 1.  Some javascript:/navigation web platform test fixups

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

::: testing/web-platform/tests/html/browsers/browsing-the-web/navigating-across-documents/012.html
@@ +4,5 @@
>  <script src="/resources/testharnessreport.js"></script>
>  <div id="log"></div>
>  <iframe id="test" name="test"></iframe>
> +<!-- XXX: What is this test trying to do?  It's navigating the subframe, but
> +     doing a write() to _this_ document, and the "javascript:" in there it

s/in there it/in there is/
Attachment #8746155 - Flags: review?(peterv) → review+
Comment on attachment 8746249 [details] [diff] [review]
part 2.  Change javascript: evaluation to ignore the return value if it's not a string

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

::: dom/jsurl/nsJSProtocolHandler.cpp
@@ +279,1 @@
>          return NS_ERROR_DOM_RETVAL_UNDEFINED;

Hmm, I guess we can't just return NS_ERROR_MALFORMED_URI because nsJSChannel::AsyncOpen special-cases NS_ERROR_DOM_RETVAL_UNDEFINED? NS_ERROR_DOM_RETVAL_UNDEFINED means something returned undefined, which isn't necessarily the case here, right?
Attachment #8746249 - Flags: review?(peterv) → review+
Attachment #8746157 - Flags: review?(peterv) → review+
> s/in there it/in there is/

Yep, done.

> I guess we can't just return NS_ERROR_MALFORMED_URI because nsJSChannel::AsyncOpen special-cases NS_ERROR_DOM_RETVAL_UNDEFINED

Yes.

> NS_ERROR_DOM_RETVAL_UNDEFINED means something returned undefined, which isn't necessarily the case here, right?

NS_ERROR_DOM_RETVAL_UNDEFINED exists solely to make this code work.  It means whatever it needs to mean here.  I could rename it to NS_ERROR_JAVASCRIPT_URL_NOT_RESULTING_IN_DOCUMENT or something if you'd like.
Flags: needinfo?(peterv)
Flags: needinfo?(peterv)
Depends on: 1306472
Depends on: 1308133
Depends on: 1307420
Given that the spec this was based on is known-bogus (see <https://github.com/whatwg/html/issues/1895>) and apparently didn't follow other browsers _either_ (see <https://github.com/whatwg/html/pull/1107#issuecomment-253558923>) I'm going to back this out.  Once the spec gets sorted out, I'll think about implementing whatever it ends up saying.
Backed out on inbound.
Resolution: FIXED → INVALID
Either this bug or bug 1270349 regressed being able to upload photos to dpreview.com photography community forums. See bug 1310414.
Bug 1477821 tracks updating to the spec once that's sorted out.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.