Closed Bug 891340 Opened 10 years ago Closed 10 years ago

Make Range.collapse optional and default to false

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: annevk, Assigned: annevk)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

attempt 2.1 [r=baku]
1.84 KB, patch
annevk
: review+
Details | Diff | Splinter Review 
This should be a fairly trivial IDL change and would be consistent with other boolean arguments we've made optional. http://dom.spec.whatwg.org/#range
Attached patch attempt 1 (obsolete) — — Splinter Review 
Assignee: mounir → annevk

        Attachment #772634 -
        Flags: review?(amarchesini)

    
    

    
  
Comment on attachment 772634 [details] [diff] [review]
attempt 1

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

Congrats! Your first r+!
I sent your patch to try. If it's green I land it.

https://tbpl.mozilla.org/?tree=Try&rev=f7e9f24d8f49

        Attachment #772634 -
        Flags: review?(amarchesini) → review+

    
    

    
  
Needs a test!

    
    

    
  
A test should be added to dom/ranges/Range-collapse.html in https://github.com/w3c/web-platform-tests It seems that can be done separately.

    
    

    
  
Comment on attachment 772634 [details] [diff] [review]
attempt 1

There is something orange on try.

        Attachment #772634 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        attempt 2 (obsolete)
        — 
          — Splinter Review
      

    


  
Bah.

        Attachment #772634 -
        Attachment is obsolete: true

        Attachment #772817 -
        Flags: review?(amarchesini)

    
    

    
  
Comment on attachment 772817 [details] [diff] [review]
attempt 2

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

https://tbpl.mozilla.org/?tree=Try&rev=c11f86a3a8eb

        Attachment #772817 -
        Flags: review?(amarchesini) → review+

    
  
Keywords: checkin-needed

    
    

    
  


  

        Attachment #772817 -
        Attachment is obsolete: true

        Attachment #772934 -
        Flags: review+

    
  

        Attachment #772934 -
        Attachment description: attempt 2.1 → attempt 2.1 [r=baku]

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/3537ffb67d4b
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

    
    

    
  
The current WebIDL parser doesn't support optional arguments.
https://tbpl.mozilla.org/php/getParsedLog.php?id=26263236&tree=Mozilla-Inbound#error1
Even worse, the current testharness doesn't catch the error. So the test is effectively disabled.
https://bugzilla.mozilla.org/show_bug.cgi?id=885107#c40

    
    

    
  
You mean the WebIDL parser for tests, right? Gecko's WebIDL parser sure supports optional.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #15)
> You mean the WebIDL parser for tests, right? Gecko's WebIDL parser sure
> supports optional.

Yes, I meant dom/imptests/WebIDLParser.js.
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.