Closed Bug 1268047 Opened 5 years ago Closed 5 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
normal

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.
Depends on: 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.