Closed Bug 1098412 Opened 10 years ago Closed 7 years ago

Remove __iterator__ and the legacy Iterator constructor

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: evilpie, Assigned: emk)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(4 files, 4 obsolete files)

Bug 1098412 - Remove and update tests that use __iterator__.
59 bytes, text/x-review-board-request
luke
: review+
Details
Bug 1098412 - Remove and update tests that use the legacy Iterator constructor.
59 bytes, text/x-review-board-request
luke
: review+
Details
Bug 1098412 - Remove __iterator__ implementation.
59 bytes, text/x-review-board-request
luke
: review+
Details
Bug 1098412 - Remove the legacy Iterator constructor.
59 bytes, text/x-review-board-request
luke
: review+
Details 
The custom __iterator__ hook makes implementing [[Enumerate]] slightly more annoying. Let's try to remove it.

Telemetry indicates between 0-3 uses, but I am not sure what his measures. mxr mostly shows hits in the addon-sdk, which might be slightly annoying, when it breaks users.

http://telemetry.mozilla.org/#filter=nightly%2F36%2FSIMPLE_MEASURES_JS_CUSTOMITER&aggregates=multiselect-all!Submissions!Mean!5th%20percentile!25th%20percentile!median!75th%20percentile!95th%20percentile&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph
Blocks: 1091900
Depends on: 1098617
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Keywords: addon-compat
Depends on: 1099329
Depends on: 1099455
Blocks: 1103158
Keywords: site-compat
Depends on: 1127084
No longer depends on: 1127084
Blocks: 1112564
After the removal of Reflect.enumerate this isn't really that important anymore for ES6.
No longer blocks: es6
Attached patch Remove __iterator__ implementation. (obsolete) — — Splinter Review 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Remove and update tests that use __iterator__. (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Remove the legacy Iterator constructor. (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
Let's see how badly removing __iterator__ and Iterator breaks things: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dd4314da721d1ba5819230190613b003fbb17f9

    
    

    
  
Is there any progress?

    
    

    
  
Shu actually left, maybe you can pick this up?

    
    

    
  
OK, taking.
Assignee: nobody → VYV03354

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
  

        Attachment #8879343 -
        Attachment is obsolete: true

    
  

        Attachment #8879344 -
        Attachment is obsolete: true

    
  

        Attachment #8879345 -
        Attachment is obsolete: true

    
  

        Attachment #8879346 -
        Attachment is obsolete: true

    
  
Status: NEW → ASSIGNED
Summary: Remove __iterator__ → Remove __iterator__ and the legacy Iterator constructor

    
    

    
  
Comment on attachment 8901428 [details]
Bug 1098412 - Remove __iterator__ implementation.

https://reviewboard.mozilla.org/r/172918/#review178546

::: js/src/jsiter.cpp:951
(Diff revision 1)
>      if (flags == 0 || flags == JSITER_ENUMERATE) {
>          if (MOZ_UNLIKELY(obj->is<ProxyObject>()))
>              return Proxy::enumerate(cx, obj);
>      }
>  
>      RootedObject res(cx);

This no longer needs to be a root. Please change this to `JSObject* res` and move it right before:

if (flags & JSITER_FOREACH) {

Also, please remove iteratorIntrinsic from vm/CommonPropertyNames.h That will catch a few more places where you can remove code, like CanStoreInIteratorCache :)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8901428 [details]
Bug 1098412 - Remove __iterator__ implementation.

https://reviewboard.mozilla.org/r/172918/#review178546

> This no longer needs to be a root. Please change this to `JSObject* res` and move it right before:
> 
> if (flags & JSITER_FOREACH) {
> 
> Also, please remove iteratorIntrinsic from vm/CommonPropertyNames.h That will catch a few more places where you can remove code, like CanStoreInIteratorCache :)

Thsnks for the comment, fixed.

    
    

    
  
Comment on attachment 8901426 [details]
Bug 1098412 - Remove and update tests that use __iterator__.

https://reviewboard.mozilla.org/r/172914/#review178646

        Attachment #8901426 -
        Flags: review?(luke) → review+

    
    

    
  
Comment on attachment 8901427 [details]
Bug 1098412 - Remove and update tests that use the legacy Iterator constructor.

https://reviewboard.mozilla.org/r/172916/#review178648

        Attachment #8901427 -
        Flags: review?(luke) → review+

    
    

    
  
Comment on attachment 8901428 [details]
Bug 1098412 - Remove __iterator__ implementation.

https://reviewboard.mozilla.org/r/172918/#review178650

        Attachment #8901428 -
        Flags: review?(luke) → review+

    
    

    
  
Comment on attachment 8901429 [details]
Bug 1098412 - Remove the legacy Iterator constructor.

https://reviewboard.mozilla.org/r/172920/#review178652

\o/  Thanks for finishing this!

        Attachment #8901429 -
        Flags: review?(luke) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/1643079d7661
Remove and update tests that use __iterator__. r=luke
https://hg.mozilla.org/integration/autoland/rev/8281805e7c3c
Remove and update tests that use the legacy Iterator constructor. r=luke
https://hg.mozilla.org/integration/autoland/rev/e9a0298824d6
Remove __iterator__ implementation. r=luke
https://hg.mozilla.org/integration/autoland/rev/f4da55f291cb
Remove the legacy Iterator constructor. r=luke

    
    

    
  
Backed out in https://hg.mozilla.org/integration/autoland/rev/eb4a7917f3e1 for failing the devtools test browser_webconsole_bug_632347_iterators_generators.js  like https://treeherder.mozilla.org/logviewer.html#?job_id=126571859&repo=autoland

Horrifyingly enough, that test is "skip-if = e10s" which now means "only run on Win7 debug" so other than running locally, on try you have to do Win7 debug to see it run.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/929a062222d8
Remove and update tests that use __iterator__. r=luke
https://hg.mozilla.org/integration/autoland/rev/8f1c6864d359
Remove and update tests that use the legacy Iterator constructor. r=luke
https://hg.mozilla.org/integration/autoland/rev/24297940e626
Remove __iterator__ implementation. r=luke
https://hg.mozilla.org/integration/autoland/rev/fce9e9b0be2b
Remove the legacy Iterator constructor. r=luke

    
  
Depends on: 1395366
No longer depends on: 1395366
Depends on: 1461324

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: