Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added .webidl files. #22070

Closed
wants to merge 12 commits into from
Closed

Added .webidl files. #22070

wants to merge 12 commits into from

Conversation

@PrayaniSingh0106
Copy link

PrayaniSingh0106 commented Nov 1, 2018

@maharsh312 @Chiaggs
Added 3 file as following:

  1. OffscreenCanvasRenderingContext2d.webidl
  2. OffscreenCanvas.webidl
  3. resources/prefs.json

r? @jdm

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

maharsh312 and others added 6 commits Nov 1, 2018
@highfive
Copy link

highfive commented Nov 1, 2018

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

@highfive
Copy link

highfive commented Nov 1, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/OffscreenCanvas.webidl, components/script/dom/webidls/OffscreenCanvasRenderingContext2d.webidl
  • @KiChjang: components/script/dom/webidls/OffscreenCanvas.webidl, components/script/dom/webidls/OffscreenCanvasRenderingContext2d.webidl
@highfive
Copy link

highfive commented Nov 1, 2018

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
Copy link
Member

jdm left a comment

This won't be able to merge until there's a corresponding Rust implementation. The bindings that are generated from the WebIDL file will complain, otherwise.

attribute [EnforceRange] unsigned long long height;

OffscreenRenderingContext? getContext(OffscreenRenderingContextId contextId, optional any options = null);
ImageBitmap transferToImageBitmap();

This comment has been minimized.

Copy link
@jdm

jdm Nov 1, 2018

Member

This will need to be commented out until we also add the ImageBitmap interface. Servo currently does not implement it.

"dom.compositionevent.enabled": false,
"dom.customelements.enabled": true,
"dom.forcetouch.enabled": false,
"dom.gamepad.enabled": false,
"dom.microdata.testing.enabled": true,
"dom.mouseevent.which.enabled": false,
"dom.mutation_observer.enabled": true,
"dom.offscreen_canvas.enabled": true,

This comment has been minimized.

Copy link
@jdm

jdm Nov 1, 2018

Member

We don't actually want to enable this preference by default until the implementation is ready.

@maharsh312
Copy link
Contributor

maharsh312 commented Nov 2, 2018

@jdm Can you tell us which variables would be part of structure of OffscreenCanvas apart from height and width?

@jdm
Copy link
Member

jdm commented Nov 2, 2018

You will also need to store a context member similar to https://github.com/servo/servo/blob/master/components/script/dom/htmlcanvaselement.rs#L58 . Apart from that, I don't know that there will need to be any other members for OffscreenCanvas.

@Chiaggs
Copy link

Chiaggs commented Nov 5, 2018

#[dom_struct]
pub struct OffscreenCanvas{
  height: u64,
  width: u64,
  context: DomRefCell<Option<CanvasContext>>,
}

impl OffscreenCanvas{
  pub fn new_inherited(height: u64,width: u64) -> OffscreenCanvas {
      OffscreenCanvas {
          height: height,
          width: width,
          context: DomRefCell::new(None),
      }
  }

  pub fn new(global: &GlobalScope,height: u64,width: u64) -> DomRoot<OffscreenCanvas> {
      reflect_dom_object(box OffscreenCanvas::new_inherited(), global, OffscreenCanvas::Bindings::Wrap)
  }

  pub fn Constructor (global: &GlobalScope,height: u64,width: u64) -> Fallible<DomRoot<OffscreenCanvas>> {
      //step 1
      let offscreencanvas = OffscreenCanvas::new(global);
      //step 2

      //step 3
      Ok(offscreencanvas);
  }
}

@jdm Apart from step 2 and get context, does this code for OffscreenCanvas.rs file looks good to you ?

@jdm
Copy link
Member

jdm commented Nov 5, 2018

You are missing a reflector field in OffscreenCanvas. That stores the pointer to the JS object that represents this Rust object.

@maharsh312
Copy link
Contributor

maharsh312 commented Nov 9, 2018

@jdm For initial step, which functionality should we implement for "offscreencanvasrenderingcontext2d.rs" ?

@jdm
Copy link
Member

jdm commented Nov 9, 2018

The initial step only requires an implementation for OffscreenCanvas.getContext and the OffscreenCanvas constructor. transferToImageBitmap and convertToBlob can remain commented out. The OffscreenCanvasRenderingContext2D interface should have the canvas attribute implemented, but the various includes lines that add 2d drawing APIs can remain commented out. The work to share that code with CanvasRenderingContext2D is part of the next steps in the project. Does that make sense?

@maharsh312
Copy link
Contributor

maharsh312 commented Nov 9, 2018

The initial step only requires an implementation for OffscreenCanvas.getContext and the OffscreenCanvas constructor. transferToImageBitmap and convertToBlob can remain commented out. The OffscreenCanvasRenderingContext2D interface should have the canvas attribute implemented, but the various includes lines that add 2d drawing APIs can remain commented out. The work to share that code with CanvasRenderingContext2D is part of the next steps in the project. Does that make sense?

Yes it makes sense. What about "commit()" in OffscreenCanvasRenderingContext2D? Should it be implemented?

@jdm
Copy link
Member

jdm commented Nov 9, 2018

I don't think it makes sense to implement it separately from the actual rendering.

@maharsh312
Copy link
Contributor

maharsh312 commented Nov 10, 2018

@jdm Can you review this code. We are getting lots of error and are stuck. It would be great if you can suggest changes.

use dom::bindings::codegen::Bindings::OffscreenCanvasBinding::{OffscreenCanvasMethods, Wrap as OffscreenCanvasWrap};
use dom::bindings::codegen::Bindings::OffscreenCanvasBinding;
use dom::bindings::codegen::UnionTypes;
use dom::bindings::error::{Error, Fallible};
use dom::bindings::reflector::{DomObject, Reflector, reflect_dom_object};
use std::ptr;
use dom::bindings::root::{DomRoot, Dom};
use std::cell::Ref;
use dom::bindings::str::DOMString;
use dom::globalscope::GlobalScope;
use dom::htmlcanvaselement::{CanvasContext, HTMLCanvasElement};
use dom_struct::dom_struct;
use dom::bindings::cell::DomRefCell;
use ref_filter_map;
use dom::offscreencanvasrenderingcontext2d::OffscreenCanvasRenderingContext2D;
use js::rust::HandleValue;
use js::jsapi::JSContext;
use dom::node::{Node, window_from_node};


pub enum OffscreenCanvasContext {
    Context2D(Dom<OffscreenCanvasRenderingContext2D>),
    //WebGL(Dom<WebGLRenderingContext>),
    //WebGL2(Dom<WebGL2RenderingContext>),
}

#[dom_struct]
pub struct OffscreenCanvas{
    reflector_: Reflector,
    height: u64,
    width: u64,
    context: DomRefCell<Option<OffscreenCanvasContext>>,
    placeholder: Option<Dom<HTMLCanvasElement>>,
}

impl OffscreenCanvas{
    pub fn new_inherited(height: u64, width: u64, placeholder: Option<&HTMLCanvasElement>) -> OffscreenCanvas {
        OffscreenCanvas {
            reflector_: Reflector::new(),
            height: height,
            width: width,
            context: DomRefCell::new(None),
            placeholder: placeholder.map(|canvas| Dom::from_rooted(canvas)),
        }
    }

    pub fn new(global: &GlobalScope, height: u64, width: u64, placeholder: Option<&HTMLCanvasElement>) -> DomRoot<OffscreenCanvas> {
        reflect_dom_object(Box::new(OffscreenCanvas::new_inherited(height,width,placeholder)), global, OffscreenCanvasWrap)
    }

    pub fn Constructor (global: &GlobalScope, height: u64, width: u64) -> Fallible<DomRoot<OffscreenCanvas>> {
        //step 1
        let offscreencanvas = OffscreenCanvas::new(global,height,width,None);
        //step 2

        if offscreencanvas.context.borrow().is_some() {
            return Err(Error::InvalidState);
        }

        offscreencanvas.height = height;
        offscreencanvas.width = width;

        offscreencanvas.placeholder = None;

        //step 3
        Ok(offscreencanvas)
    }

    /*pub fn context(&self) -> Option<Ref<OffscreenCanvasContext>> {
        ref_filter_map::ref_filter_map(self.context.borrow(), |ctx| ctx.as_ref())
    }*/

    fn get_or_init_2d_context(&self) -> Option<DomRoot<OffscreenCanvasRenderingContext2D>> {
        if let Some(ctx) = self.context() {
            return match *ctx {
                OffscreenCanvasContext::Context2d(ref ctx) => Some(DomRoot::from_ref(ctx)),
                _ => None,
            };
        }
        let window = window_from_node(self);
        let size = self.get_size();
        let context = OffscreenCanvasRenderingContext2D::new(window.upcast::<GlobalScope>(), self, size);
        *self.context.borrow_mut() = Some(OffscreenCanvasContext::Context2d(Dom::from_ref(&*context)));
        Some(context)
    }

}


impl OffscreenCanvasMethods for OffscreenCanvas{
    #[allow(unsafe_code)]
    unsafe fn GetContext(&self,cx: *mut JSContext, contextID: DOMString, options: HandleValue) -> Option<UnionTypes::OffscreenCanvasRenderingContext2DOrWebGLRenderingContextOrWebGL2RenderingContext> {

        //let options =
        if !options.is_object() {
            options = HandleValue::null();
        }



        if(self.context.is_none())
        {
            if(contextID == "2d")
            {
                //self.get_or_init_2d_context();
            }
        }

    }



    fn Width(&self) -> u64 {
        return self.width;
    }
    fn SetWidth(&self, value: u64) -> () {
        self.width = value;
    }
    fn Height(&self) -> u64 {
        return self.height;
    }
    fn SetHeight(&self, value: u64) -> () {
        self.height = value;
    }
}

Errors: https://pastebin.com/jvN1u4NP

code changes to add offscreen canvas
Offscreen canvas
maharsh312 and others added 2 commits Nov 10, 2018
@jdm
Copy link
Member

jdm commented Nov 11, 2018

@maharsh312 It would be useful if you specified the errors you are observing.

@jdm
Copy link
Member

jdm commented Nov 11, 2018

Sorry, I missed the link to the errors.

@jdm
Copy link
Member

jdm commented Nov 11, 2018

  • For the MallocSizeOf, you need to add #[derive(MallocSizeOf)] for the OffscreenCanvasContext type. Similarly for the trace error, you need to derive JSTraceable for OffscreenCanvasContext. * Instead of Dom::from_rooted use Dom::from_ref (my mistake).
  • For the error about window_from_node, you can use self.global() to obtain a GlobalScope object instead, since that is what you actually need.
  • For errors like the missing is_none() method, you need to call borrow() or borrow_mut() on a DomRefCell<T> value in order to obtain a &T pointer to the actual wrapped value.
  • For the error about the Reflector member in OffscreenCanvas, you actually need a eventtarget: EventTarget member instead of the Reflector, since the WebIDL declares that the parent interface is EventTarget. You can initialize this by calling EventTarget::new_inherited (which means "construct a new instance of this type that is used to simulate C++ inheritance).
  • I think the remaining errors are caused by calling methods that don't exist or using the wrong name for case-sensitive enum variants.
Followed Josh's review 1 and 4
@jdm
Copy link
Member

jdm commented Nov 16, 2018

I'm closing this PR under the assumption that it's a duplicate of #22168.

@jdm jdm closed this Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.